-
Notifications
You must be signed in to change notification settings - Fork 20
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
Semconv code generation prototype #40
Conversation
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.
I have a slight preference for having two separate artifacts, one for stable attributes and one for (the last 5) experimental namespaced attributes
// DO NOT EDIT, this is an Auto-generated file from | ||
// buildscripts/templates/SemanticAttributes.java.j2 | ||
@SuppressWarnings("unused") | ||
public final class CloudeventsAttributes { |
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.
Cloudevents
-> CloudEvents
?
Originally cloudevents
in the spec, so we can't CamelCase them automatically.
There are more of this like Opentracing
, Graphgl
- do we want to override it?
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.
TODO: add custom overrides
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.
I'm not sure about custom overrides. We currently translate snake_case
to UpperCamel
, and the problem here is that we are presented with a value that is normally thought of as two words subject to camel case (i.e. CloudEvents
) as a single word (i.e. cloudevents
).
If we add a set of special cases, to replace cloudevents
with CloudEvents
, then we susceptible to conflicts in the future if a word comes along which is matches our override. I.e. if later cloud_events
comes along, then both cloud_events
and cloudevents
map to CloudEvents
.
Better I think to not have special cases, and fix upstream in semantic conventions in obvious cases.
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, I agree. I've been thinking about it more:
If we do overrides, they
- should be centralized
- someone who introduces new semconv should decide. Maybe yaml should have a
case_sensitive_name
field?
Otherwise it becomes unmanageable.
My plan for now is to play with python semconv generation a bit to get a different perspective and then suggest changes to semconv to handle it.
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.
Closing as this is superseded by #45. |
This is a prototype (related to open-telemetry/semantic-conventions#551) of new code-gen approach:
io.opentelemetry.semconv
io.opentelemetry.semconv.vA_B_C
The process of codegen might looks like:
Semantic|ResourceAttributes
with all deprecated things, delete at some point in the futureAs a result, we only need to care about back-compat when a stable attribute changes in a breaking manner.
Multiple instrumentation libs can send different semconv versions at once.
Bloating: is indeed a problem
TODO: