-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #11064 +/- ##
==========================================
- Coverage 92.23% 92.20% -0.03%
==========================================
Files 405 414 +9
Lines 19089 19718 +629
==========================================
+ Hits 17606 18181 +575
- Misses 1123 1166 +43
- Partials 360 371 +11 ☔ 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. |
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