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

ServiceGraph: support specifying time unit and histogram type, and buckets #27881

Closed
cyrille-leclerc opened this issue Oct 20, 2023 · 7 comments

Comments

@cyrille-leclerc
Copy link
Member

cyrille-leclerc commented Oct 20, 2023

Component(s)

connector/servicegraph

Is your feature request related to a problem? Please describe.

I want to use simultaneously the Service Graph Connector and the Span Metrics Connector with a Prometheus backend and to have both processors produce measurements in s rather than ms and have the same histograms histogram characteristics (units and time buckets).

Describe the solution you'd like

Add a histogram/timeunit configuration option in the Service Graph Connector and be consistent with the Span Metrics Connector histogram/timeunit configuration setting.

A lower priority would be to add support for specifying the histogram type explicit|exponential and bucket details similar to what the Span Metrics Connector offers

Describe alternatives you've considered

Convert Service Graph Connector metrics from ms to s in a downstream OTel Collector processor.

Additional context

See also:

@cyrille-leclerc cyrille-leclerc added enhancement New feature or request needs triage New item requiring triage labels Oct 20, 2023
@cyrille-leclerc cyrille-leclerc changed the title ServiceGraph: support specifying time unit and histogram type and buckets ServiceGraph: support specifying time unit and histogram type, and buckets Oct 20, 2023
@github-actions
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@crobert-1
Copy link
Member

crobert-1 commented Nov 1, 2023

Please note that my comments could be overruled by the code owners, I don't have a lot of experience here.

I think this is a valid request, but as described below this would be a heavily involved breaking change.

  1. All of the relevant logic is still in the servicegraph processor, so we'd be adding logic to the deprecated component.
  2. Also, the histogram metric names themselves have units as suffixes:
traces_service_graph_request_server_seconds
traces_service_graph_request_client_seconds

It would be a breaking change to rename the metrics to no longer include the seconds suffix.

  1. Variables are also created and set in multiple places across the processor, and also include the suffix Seconds more often than not. This is more complexity to deal with in implementation.

Also, a clarification on your comment and have the same histograms. Do you want the servicegraph connector to create the histogram metrics with the exact same names as the spanmetrics connector? Or could you clarify what exactly you're looking for there?

@crobert-1 crobert-1 removed the needs triage New item requiring triage label Nov 1, 2023
@cyrille-leclerc
Copy link
Member Author

Thanks @crobert-1!

Also, a clarification on your comment and have the same histograms. Do you want the servicegraph connector to create the histogram metrics with the exact same names as the spanmetrics connector? Or could you clarify what exactly you're looking for there?

Great catch, I clarified, I meant the same characteristics (time unit and buckets).

I see a broader topic of aligning better the Span Metrics and Service Graph connectors but it's a longer term topic initiated by @jpkrohling in:

@crobert-1
Copy link
Member

Thanks for the reference to #26648, I wasn't aware of that effort. Good to know!

Copy link
Contributor

github-actions bot commented Jan 2, 2024

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the Stale label Jan 2, 2024
@crobert-1 crobert-1 removed the Stale label Jan 2, 2024
Copy link
Contributor

github-actions bot commented Mar 4, 2024

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the Stale label Mar 4, 2024
Copy link
Contributor

github-actions bot commented May 3, 2024

This issue has been closed as inactive because it has been stale for 120 days with no activity.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants