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

New semconvgen templates prototype #5156

Closed

Conversation

lmolkova
Copy link

Prototype for new semconv generation approach (that groups attributes by namespace and stability vs semconv)
open-telemetry/semantic-conventions#551

Changes

  • Breaks down attribute definitions into individual files based on their root namespace
  • Defines experimental attributes in the corresponding vX_Y_Z_Experimental namespace
  • Demonstrates how metric definitions can be auto-generated

Merge requirement checklist

  • CONTRIBUTING guidelines followed (nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

/// <summary>
/// Describes semantic conventions for attributes in the <c>cloudevents</c> namespace.
/// </summary>
public static class CloudeventsAttributes
Copy link
Author

Choose a reason for hiding this comment

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

cloudevents, graphql, webengine, opentracing, etc cannot be automatically CamelCased, so need to figure out something on the semconv side to fix it across languages

Copy link
Author

Choose a reason for hiding this comment

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

/// <remarks>
/// When observed from the server side, and when communicating through an intermediary, <c>client.address</c> SHOULD represent the client address behind any intermediaries, for example proxies, if it&amp;#39;s available.
/// </remarks>
public const string ClientAddress = "client.address";
Copy link
Author

Choose a reason for hiding this comment

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

Q:
ClientAttributes.ClientAddress vs ClientAttributes.Address ?

/// <summary>
/// Describes semantic conventions for attributes in the <c>other</c> namespace.
/// </summary>
public static class OtherAttributes
Copy link
Author

Choose a reason for hiding this comment

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

todo on the spec side: all attributes must have a namespace

/// <remarks>
/// The size of the request payload body in bytes. This is the number of bytes transferred excluding headers and is often, but not always, present as the <a href="https://www.rfc-editor.org/rfc/rfc9110.html#field.content-length">Content-Length</a> header. For requests using transport encoding, this should be the compressed size.
/// </remarks>
public static Histogram<double> CreateHttpClientRequestBodySize(Meter meter)
Copy link
Author

Choose a reason for hiding this comment

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

value type is not part of yaml - open-telemetry/semantic-conventions#591, so it's not fully possible yet

-D filter=is_experimental `
-D pkg=OpenTelemetry.SemanticConventions.${SPEC_VERSION_ESCAPED}

#experimental metrics
Copy link
Author

Choose a reason for hiding this comment

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


#experimental attributes
Copy link
Author

Choose a reason for hiding this comment

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

it's easy to filter stable/experimental and move them into different artifacts, use different templates, add [Experimental] attribute, etc

/// Instrumentations for specific web frameworks that consider HTTP methods to be case insensitive, SHOULD populate a canonical equivalent.
/// Tracing instrumentations that do so, MUST also set <c>http.request.method_original</c> to the original value.
/// </remarks>
public const string HttpRequestMethod = "http.request.method";
Copy link
Author

Choose a reason for hiding this comment

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

Would we want multiple layers like HttpAttributes.Request?

@martinjt
Copy link
Member

I'm slightly worried about the idea that "use whatever is the latest", versus pinning to a specific version.

The problem I see is that if I use:

OpenTelemetry.SemanticConventions.HttpAttributes.HttpRequestMethod and a V2 of the semantic conventions changes that, I'm going to automatically inherit that change in my library if the consumer is using a V2 version.

I think having the ability to have V1_23_1 namespace, and the default you've provided allows authors to opt-in to "whatever is new", or "this specific version of the conventions".

(missed the SIG, sorry if it was discussed there)

Copy link
Contributor

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Dec 20, 2023
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale Issues and pull requests which have been flagged for closing due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants