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

metrics: add an interface to get the metrics definitions #155

Merged
merged 2 commits into from
Sep 13, 2024

Conversation

Gandem
Copy link
Contributor

@Gandem Gandem commented Sep 13, 2024

Fixes #149

This will enable profilers using the ebpf profiler as a library to export some of the internal metrics of the profiler. For example, this should allow getting more data on unwinding errors for interpreted languages.

For the interface, I added a GetDefinitions() method in the metrics package, which would require exposing the metrics package in #132, happy to move this elsewhere though if we think this is not the best place / API for this.

@Gandem Gandem requested review from a team September 13, 2024 12:38
Copy link
Member

@felixge felixge 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. 🙇

metrics/metrics_test.go Outdated Show resolved Hide resolved
Comment on lines +168 to +172
err := dec.Decode(&definitions)
if err != nil {
return nil, err
}
return definitions, nil
Copy link
Member

Choose a reason for hiding this comment

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

NIT: This could be simplified:

Suggested change
err := dec.Decode(&definitions)
if err != nil {
return nil, err
}
return definitions, nil
return definitions, dec.Decode(&definitions)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIUC, unfortunately this linter rule disallows this: https://go-critic.com/overview.html#evalorder

Copy link
Contributor

@athre0z athre0z left a comment

Choose a reason for hiding this comment

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

LGTM -- thank you!

@florianl florianl merged commit ce5afa2 into open-telemetry:main Sep 13, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose metric information
4 participants