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

reporter: use semantic convention to report build_id #153

Merged
merged 1 commit into from
Sep 20, 2024
Merged

Conversation

florianl
Copy link
Contributor

open-telemetry/semantic-conventions#1329 got merged, which introduces a semantic convention for build_ids. Use this semantic convention in favor of BuildIdKind, that will be dropped with open-telemetry/opentelemetry-proto#584.

@florianl florianl requested review from a team September 12, 2024 08:17
open-telemetry/semantic-conventions#1329 got
merged, which introduces a semantic convention for build_ids. Use this
semantic convention in favor of BuildIdKind, that will be dropped with
open-telemetry/opentelemetry-proto#584.

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
Comment on lines 524 to 529
// AttributeUnits - Optional element we do not use.
// LinkTable - Optional element we do not use.
// DropFrames - Optional element we do not use.
// KeepFrames - Optional element we do not use.
// TimeNanos - Optional element we do not use.
// DurationNanos - Optional element we do not use.
// PeriodType - Optional element we do not use.
// Period - Optional element we do not use.
// Comment - Optional element we do not use.
// DefaultSampleType - Optional element we do not use.
Copy link
Member

Choose a reason for hiding this comment

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

I'd also remove all of these, they're just noise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No strong feelings here. These comments helped to implement the protocol and to check which fields are implemented and which not. When working on this change, I just noticed, that some of these comments are no longer correct.

Copy link
Contributor

@fabled fabled left a comment

Choose a reason for hiding this comment

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

LGTM. Do we want to also support sending the Go build id as per spec?

@florianl
Copy link
Contributor Author

Do we want to also support sending the Go build id as per spec?

As there is a higher demand for process.executable.build_id.gnu and process.executable.build_id.profiling I priorized their implementation with this change. process.executable.build_id.go is planned to follow.

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.

3 participants