-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Migrate collector semconv codegen to weaver #11064
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #11064 +/- ##
=======================================
Coverage 91.55% 91.55%
=======================================
Files 424 424
Lines 20199 20199
=======================================
Hits 18494 18494
Misses 1320 1320
Partials 385 385 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weaver/template changes LGTM!
// Type: string | ||
// Requirement Level: Optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requirement level disappeared
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - There is not such things are requirement level on the attribute registry, that's signal specific.
I'm not even sure where it was pulling requirement level from before, but I call that out in the PR description.
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
NOTE: This PR is (currently) an example for discussion on the evolution of semconv codegen in the collector.
I regenerated v1.27.0 semconv package so you can get a feel for the changes - I can revert that if we go this route.
Description
Migrate semconv codegen to weaver.
Discovered a few things with codegen in the collector that may be problematic going forward, but did the bare minimum here:
Link to tracking issue
Tracked at: open-telemetry/weaver#227
Testing
I manually ran the codegen against latest semconv and compared agianst current versions. I can run this codegen in existing directories for clarity.
Documentation
Documentation remains unchanged for codegen. I can add links to weaver documentation if needed.
Open Questions / Concerns