-
Notifications
You must be signed in to change notification settings - Fork 289
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
[repo] Move OpenTelemetry.SemanticConventions project from main repo #1672
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1672 +/- ##
==========================================
- Coverage 73.91% 72.55% -1.37%
==========================================
Files 267 240 -27
Lines 9615 9261 -354
==========================================
- Hits 7107 6719 -388
- Misses 2508 2542 +34
Flags with carried forward coverage won't be shown. Click here to find out more. |
test/OpenTelemetry.AotCompatibility.TestApp/OpenTelemetry.AotCompatibility.TestApp.csproj
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.SemanticConventions/OpenTelemetry.SemanticConventions.csproj
Show resolved
Hide resolved
Would you mind adding a
|
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.
Consider adding CI/infrqastructure code similar to 9fbfdf2
Also it will be great to have component owner under .github repository for this project.
src/OpenTelemetry.SemanticConventions/SemanticConventionsAttributes.cs
Outdated
Show resolved
Hide resolved
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.
Having all attributes in one place would be problematic in the long term: different stability, huge file, hard to use
I suggest to reuse new approach we've implemented in Java and Python - group attributes by the root namespace following the discussion in open-telemetry/semantic-conventions#551
It's being documented here https://github.com/open-telemetry/semantic-conventions/blob/18325605937cb0a15fde89faabf48c857eab0ca5/supplementary-guidelines/semantic_conventions_code_generation.md.
Here's how Java does it https://github.com/open-telemetry/semantic-conventions-java.
This is not a requirement, and from semconv perspective we want to give language SIGs a freedom to decide how they want to do it.
Happy to help with Jinja if needed.
/// Constants for semantic attribute names outlined by the OpenTelemetry specifications. | ||
/// </summary> | ||
/// <remarks> | ||
/// Schema and specification version: {{schemaUrl}}. |
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.
Once .NET allows to set schema URL, having a single one would be problematic - different instrumentations can produce different schema versions.
public static class SemanticConventionsAttributes | ||
{ | ||
{% for attribute in attributes if attribute.is_local and not attribute.ref %} | ||
{% if not loop.first %}{{"\n"}}{% endif %} |
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.
you might want different template attributes (like http.request.header.<key>
) to appear differently than regular ones
src/OpenTelemetry.SemanticConventions/SemanticConventionsAttributes.cs
Outdated
Show resolved
Hide resolved
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.
LGTM
src/OpenTelemetry.SemanticConventions/scripts/templates/SemanticConventionsAttributes.cs.j2
Show resolved
Hide resolved
/// <summary> | ||
/// {{format_xml_doc(member.brief | render_markdown(code="<c>{0}</c>", paragraph="{0}"))}} | ||
/// </summary> | ||
public const {{ type }} {{ member.member_id | to_camelcase(True) }} = {{ print_value(type, member.value) }}; |
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.
nit: it should be possible to get rid of print_value
macro and use print_member_value
filter provided by the otel/semconvgen
public const {{ type }} {{ member.member_id | to_camelcase(True) }} = {{ print_value(type, member.value) }}; | |
public const {{ type }} {{ member.member_id | to_camelcase(True) }} = {{ print_member_value(member) }}; |
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.
Thank you!
Hi all, and @joegoldman2 I'm following up on this as a semconv maintainer and helping migrate to the new tooling (Weaver). I notice that Was this PR really updated with semconv version 1.25? Or was it just migrated as is from the main repo? (with spec version 1.13) |
For anyone wondering, the team are working on a polygot generator here. The aim being to republish this soon but with best practices of versioning them to provide a stable interface |
@ben-wilson-mews so you are already working on migrating this to weaver? I also started working on it #1944. How far long are you? I can maybe give what I have and you can continue if you want. Let me know. |
Fixes #1657.
Changes
Reintroduce
OpenTelemetry.SemanticConventions
project that has been removed with open-telemetry/opentelemetry-dotnet#5539.PR created as draft just to see if I'm going in the right direction
and because the generation script is throwing an exception at the moment.For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes