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.

Sign up for GitHub

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

Jump to bottom

Add a otelcol.connector.servicegraph component #5008

Merged
merged 4 commits into from
Sep 8, 2023
Merged

Add a otelcol.connector.servicegraph component #5008

merged 4 commits into from
Sep 8, 2023

Conversation

ptodev
Copy link
Contributor

@ptodev ptodev commented Aug 30, 2023 •
edited
Loading

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 a processor.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 the virtual_node_peer_attributes argument.

PR Checklist

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

@ptodev ptodev requested review from a team and clayton-cornell as code owners August 30, 2023 11:51
@ptodev ptodev linked an issue Aug 30, 2023 that may be closed by this pull request
Flow: otelcol.processor.servicegraph component #2300
Closed
@ptodev ptodev force-pushed the 2300-flow-otelcolprocessorservicegraph-component branch from 75bef6b to adacc95 Compare August 30, 2023 12:48
tpaschalis
tpaschalis reviewed Aug 31, 2023
View reviewed changes
Copy link
Member

@tpaschalis tpaschalis left a 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

docs/sources/flow/reference/components/otelcol.connector.servicegraph.md
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).
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor

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.

component/otelcol/connector/servicegraph/servicegraph.go Outdated Show resolved Hide resolved
component/otelcol/connector/servicegraph/servicegraph.go Outdated Show resolved Hide resolved
component/otelcol/connector/servicegraph/servicegraph.go Outdated Show resolved Hide resolved
component/otelcol/connector/servicegraph/servicegraph.go Outdated Show resolved Hide resolved
component/otelcol/connector/servicegraph/servicegraph.go Outdated Show resolved Hide resolved
component/otelcol/connector/servicegraph/servicegraph_test.go Outdated Show resolved Hide resolved
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
@ptodev ptodev force-pushed the 2300-flow-otelcolprocessorservicegraph-component branch from adacc95 to 2fc97e1 Compare August 31, 2023 09:26
@ptodev ptodev requested a review from tpaschalis August 31, 2023 09:51
tpaschalis
tpaschalis approved these changes Aug 31, 2023
View reviewed changes
Copy link
Member

@tpaschalis tpaschalis left a 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 🙌

clayton-cornell
clayton-cornell approved these changes Aug 31, 2023
View reviewed changes
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.

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
docs/sources/flow/reference/components/otelcol.connector.servicegraph.md
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).
Copy link
Contributor

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.

@tpaschalis tpaschalis force-pushed the 2300-flow-otelcolprocessorservicegraph-component branch from 731ba7b to 2110bc1 Compare September 8, 2023 14:02
@tpaschalis tpaschalis merged commit a56c84b into main Sep 8, 2023
8 checks passed
@tpaschalis tpaschalis deleted the 2300-flow-otelcolprocessorservicegraph-component branch September 8, 2023 14:16
@clayton-cornell clayton-cornell mentioned this pull request Sep 11, 2023
Merged
4 tasks
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Feb 22, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tpaschalis tpaschalis tpaschalis approved these changes

@clayton-cornell clayton-cornell clayton-cornell approved these changes

Assignees
No one assigned
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Flow: otelcol.processor.servicegraph component Flow: add OpenTelemetry Collector components
3 participants
@ptodev @tpaschalis @clayton-cornell