-
Notifications
You must be signed in to change notification settings - Fork 489
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
Add a otelcol.connector.servicegraph component #5008
Add a otelcol.connector.servicegraph component #5008
Conversation
75bef6b
to
adacc95
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, only some docs-related nits for now
Multiple `otelcol.connector.servicegraph` components can be specified by giving them | ||
different labels. | ||
|
||
This component is based on [Grafana Tempo's service graph processor](https://github.com/grafana/tempo/tree/main/modules/generator/processor/servicegraphs). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to understand, is it both based on Tempo's service graph processor, that in turn is a wrapper over the servicegraph connector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otelcol.connector.servicegraph
is a wrapper over Collector's servicegraph
connector, which on the other hand is inspired by the Tempo service graph processor.
As far as I can tell, the Otel component doesn't import any Tempo code.
I think the reason why they mention Tempo in the Otel wiki page is because (I think) the Otel component inherits the metric names which the Tempo processor in question uses. So any other UI will also have to use the same metric conventions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can then remove the Tempo reference or amend this sentence to mention the inspiration of metric names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say... only include the reference to Tempo if it helps the reader understand the topic. If it's just a nod to Tempo, or extra info that isn't really going help with understanding... then we should remove it.
docs/sources/flow/reference/components/otelcol.connector.servicegraph.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/otelcol.connector.servicegraph.md
Outdated
Show resolved
Hide resolved
adacc95
to
2fc97e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, leaving to @clayton-cornell for a final pass 🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to update the shared topic link syntax.
Added a comment about the Tempo linking.
docs/sources/flow/reference/components/otelcol.connector.servicegraph.md
Show resolved
Hide resolved
docs/sources/flow/reference/components/otelcol.connector.servicegraph.md
Show resolved
Hide resolved
Multiple `otelcol.connector.servicegraph` components can be specified by giving them | ||
different labels. | ||
|
||
This component is based on [Grafana Tempo's service graph processor](https://github.com/grafana/tempo/tree/main/modules/generator/processor/servicegraphs). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say... only include the reference to Tempo if it helps the reader understand the topic. If it's just a nod to Tempo, or extra info that isn't really going help with understanding... then we should remove it.
Co-authored-by: Paschalis Tsilias <tpaschalis@users.noreply.github.com>
731ba7b
to
2110bc1
Compare
PR Description
The new
otelcol.connector.servicegraph
component is based on the servicegraphconnector, which on the other hand is based on servicegraphprocessor.Which issue(s) this PR fixes
Fixes #2300
Fixes #2213
Notes to the Reviewer
There is one config parameter for the servicegraph connector which we cannot yet support in flow -
virtual_node_peer_attributes
. It is behind aprocessor.servicegraph.virtualNode
feature gate in the Collector. In the Agent we don't yet support enabling Otel feature gates. I will try to submit a PR to the Collector to either remove that feature gate or enable it by default, and the next time we upgrade Otel in the Agent we can add thevirtual_node_peer_attributes
argument.PR Checklist