Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(otelcol/connector/servicegraph): add virtual_node_peer_attributes setting to the component #879

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

hainenber
Copy link
Contributor

@hainenber hainenber commented May 16, 2024

PR Description

Add the virtual_node_peer_attributes setting to otelcol.connector.servicegraph. Thanks @erikbaranowski for creating the foundation already!

The default attributes are reduced to be identical to upstream

Which issue(s) this PR fixes

Closes #861

Notes to the Reviewer

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

…es` setting to the component

Signed-off-by: hainenber <[email protected]>
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@wildum wildum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@wildum
Copy link
Contributor

wildum commented May 23, 2024

waiting for @clayton-cornell feedback on the doc before merging it

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@ptodev ptodev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened a PR upstream for this config option to be changed upstream. It would be better to update Alloy straight to the new config, so let's put the PR on hold for now to see if the upstream PR gets accepted.

@hainenber
Copy link
Contributor Author

All fine by me :D

@hainenber hainenber marked this pull request as draft May 29, 2024 16:40
Copy link
Contributor

@clayton-cornell clayton-cornell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still in passive voice, but leaving it as it gets the technical point across.

@@ -107,6 +108,19 @@ Additional labels can be included using the `dimensions` configuration option:

When `metrics_flush_interval` is set to `0s`, metrics will be flushed on every received batch of traces.

`virtual_node_peer_attributes` is useful when an OTel-instrumented client sends a request to a service which is not OTel-instrumented.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`virtual_node_peer_attributes` is useful when an OTel-instrumented client sends a request to a service which is not OTel-instrumented.
`virtual_node_peer_attributes` is useful when an OTel-instrumented client sends a request to a service that is not OTel-instrumented.


If no client span is found and `virtual_node_peer_attributes` is not an empty list,
then the service span will be paired with a virtual node called `client="user"`.
This can be useful when a client which is not OTel-instrumented (like a web browser) sends a request to an OTel-instrumented service.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This can be useful when a client which is not OTel-instrumented (like a web browser) sends a request to an OTel-instrumented service.
This can be useful when a client that is not OTel-instrumented (like a web browser) sends a request to an OTel-instrumented service.

@@ -107,6 +108,19 @@ Additional labels can be included using the `dimensions` configuration option:

When `metrics_flush_interval` is set to `0s`, metrics will be flushed on every received batch of traces.

`virtual_node_peer_attributes` is useful when an OTel-instrumented client sends a request to a service which is not OTel-instrumented.
Normally, `otelcol.connector.servicegraph` wouldn't be able to pair the client span with a service span,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Normally, `otelcol.connector.servicegraph` wouldn't be able to pair the client span with a service span,
Normally, `otelcol.connector.servicegraph` wouldn't be able to pair the client span with a service span

Copy link
Contributor

This PR has not had any activity in the past 30 days, so the needs-attention label has been added to it.
If you do not have enough time to follow up on this PR or you think it's no longer relevant, consider closing it.
The needs-attention label signals to maintainers that something has fallen through the cracks. No action is needed by you; your PR will be kept open and you do not have to respond to this comment. The label will be removed the next time this job runs if there is new activity.
Thank you for your contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the virtual_node_peer_attributes setting to otelcol.connector.servicegraph
5 participants