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

Semconv code generation prototype #40

Closed
wants to merge 4 commits into from

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova commented Dec 2, 2023

This is a prototype (related to open-telemetry/semantic-conventions#551) of new code-gen approach:

  • generates stable attributes in io.opentelemetry.semconv
  • generates experimental attributes in io.opentelemetry.semconv.vA_B_C
  • groups attributes by root namespace and generates them in multiple files

The process of codegen might looks like:

  • keep old Semantic|ResourceAttributes with all deprecated things, delete at some point in the future
  • update the version
  • generate a new folder for experimental, update stable

As 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

  • the size of the artifact produced with this change is 131 KB
  • if I generate a prev version - v1.22.0, it becomes 202 KB

TODO:

  • Figure out how to populate schema URL
    • manually for now (with automated check)
    • not possible(?) with jinja, need some custom script to add new value to Java file
    • do the script
  • stable and deprecated are mutually exclusive now, so template would not work in the future if a stable attribute gets deprecated - Disambiguate attribute deprecation semantic-conventions#582

Copy link
Member

@trask trask left a 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 {
Copy link
Contributor Author

@lmolkova lmolkova Dec 7, 2023

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: add custom overrides

Copy link
Member

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.

Copy link
Contributor Author

@lmolkova lmolkova Dec 11, 2023

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jack-berg
Copy link
Member

Closing as this is superseded by #45.

@jack-berg jack-berg closed this Feb 9, 2024
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