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

[mdatagen] add CustomMetricName func #9455

Conversation

codeboten
Copy link
Contributor

This allows components to generate a consistent metric name. Doing this in mdatagen will allow us to remove the need to export methods in helpers, for example BuildCustomMetricName.

Eventually we might want to define the custom metrics for each component's internal telemetry via metadata.yaml, at which point we can remove this func. For now it allows us to clean up some more code.

This allows components to generate a consistent metric name. Doing this in mdatagen will allow us to remove
the need to export methods in helpers, for example `BuildCustomMetricName`.

Eventually we might want to define the custom metrics for each component's internal telemetry via
metadata.yaml, at which point we can remove this func. For now it allows us to clean up some more
code.

Signed-off-by: Alex Boten <[email protected]>
@codeboten codeboten requested review from a team and bogdandrutu February 2, 2024 17:15
Signed-off-by: Alex Boten <[email protected]>
Copy link

codecov bot commented Feb 2, 2024

Codecov Report

Attention: Patch coverage is 0% with 24 lines in your changes are missing coverage. Please review.

Project coverage is 90.28%. Comparing base (7abb962) to head (5d40865).
Report is 269 commits behind head on main.

Files Patch % Lines
cmd/mdatagen/internal/metadata/generated_status.go 0.00% 2 Missing ⚠️
...ardconnector/internal/metadata/generated_status.go 0.00% 2 Missing ⚠️
...ebugexporter/internal/metadata/generated_status.go 0.00% 2 Missing ⚠️
...gingexporter/internal/metadata/generated_status.go 0.00% 2 Missing ⚠️
...otlpexporter/internal/metadata/generated_status.go 0.00% 2 Missing ⚠️
...httpexporter/internal/metadata/generated_status.go 0.00% 2 Missing ⚠️
...astextension/internal/metadata/generated_status.go 0.00% 2 Missing ⚠️
...terextension/internal/metadata/generated_status.go 0.00% 2 Missing ⚠️
...gesextension/internal/metadata/generated_status.go 0.00% 2 Missing ⚠️
...tchprocessor/internal/metadata/generated_status.go 0.00% 2 Missing ⚠️
... and 2 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9455      +/-   ##
==========================================
- Coverage   90.40%   90.28%   -0.12%     
==========================================
  Files         344      344              
  Lines       18058    18082      +24     
==========================================
  Hits        16325    16325              
- Misses       1401     1425      +24     
  Partials      332      332              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

I would maybe consider connector/forward as the meter name instead, since that would nicely group all the metrics.

@codeboten
Copy link
Contributor Author

I would maybe consider connector/forward as the meter name instead, since that would nicely group all the metrics.

@bogdandrutu i think there's a few different ways we're currently customizing telemetry for internal components. In some cases, we're using a specific meter name:

https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/3952e23232bf6edd4f11d024c174d3aaf6b61a82/exporter/awss3exporter/internal/metadata/generated_status.go#L19

In others we're prefixing the component details in the name of the metric:

return componentPrefix + configType + obsmetrics.NameSep + metric

And in yet another case, we're adding specific component in a separate label:

attribute.String(obsmetrics.ExporterKey, cfg.ExporterID.String()),

I think it makes sense to consolidate these into a consistent experience for component owners are well as end users, I would suggest using issue #9315 to decide on the right path to move forward. This PR allows us to hide the details of getting a custom metric name behind a generated method internal to components rather than an exposed API in the processor helper. WDYT?

@bogdandrutu
Copy link
Member

In others we're prefixing the component details in the name of the metric:

We are doing that for legacy reasons, opencensus did not have a namespace/scope.

And in yet another case, we're adding specific component in a separate label:

I find no problem with this, since we record this from "exporterhelper" it is a fine way of doing it. We could have created different scopes per initialization or using one scope and put that as attribute. If you ask me what I would prefer is to put that in the scope, but again when that was created we had opencensus without namespace/scope so this was not an option.

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added Stale and removed Stale labels Feb 21, 2024
Copy link
Contributor

github-actions bot commented Mar 9, 2024

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added Stale and removed Stale labels Mar 9, 2024
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added Stale and removed Stale labels Mar 26, 2024
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Apr 11, 2024
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants