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

HttpSemanticConventions - update helper class #4765

Merged
merged 10 commits into from
Aug 17, 2023
Merged

HttpSemanticConventions - update helper class #4765

merged 10 commits into from
Aug 17, 2023

Conversation

TimothyMothra
Copy link
Contributor

@TimothyMothra TimothyMothra commented Aug 11, 2023

Towards #4484

When I started on this, I was working off the older version of the spec in the old repo.
The current version of the spec in the new repo changed the environment variable to a comma-separated list.

SHOULD introduce an environment variable OTEL_SEMCONV_STABILITY_OPT_IN in the existing major version which is a comma-separated list of values.
https://github.com/open-telemetry/semantic-conventions/tree/v1.21.0/docs/http

Changes

  • refactor HttpSemanticConventionsHelper
    • rewrite to parse a CSV
  • updated several projects to include Shared\Shims\NullableAttributes
  • refactor unit tests.
    • replaced InlineData with MemberData so each test can use the same collection of test cases.
    • added additional csv test cases

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)

Note about duplicate values in the Environment Variable.

Edit: 8/14: The spec has been updated to specify if both http and http/dup are specified, http/dup will take precedence.
open-telemetry/semantic-conventions#249

As of today, the only valid values are http (emit new attributes) and http/dup (emit both new and old attributes).
The spec doesn't define how to handle a scenario where a user provides both values in the CSV.
I opened an issue to get clarity: open-telemetry/semantic-conventions#240

In this scenario, Java takes a user-friendly approach of the approach of enabling both new and old attributes. I think we should do the same here.
https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/32c5d4c26708ccaa20221bb6d687f56c9efde979/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/SemconvStability.java#L26-L39

@TimothyMothra TimothyMothra requested a review from a team August 11, 2023 18:43
@codecov
Copy link

codecov bot commented Aug 11, 2023

Codecov Report

Merging #4765 (155e9c1) into main (5784b45) will increase coverage by 0.02%.
The diff coverage is 86.66%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4765      +/-   ##
==========================================
+ Coverage   82.27%   82.29%   +0.02%     
==========================================
  Files         314      314              
  Lines       12923    12930       +7     
==========================================
+ Hits        10632    10641       +9     
+ Misses       2291     2289       -2     
Files Changed Coverage Δ
src/Shared/HttpSemanticConventionHelper.cs 82.35% <86.66%> (+2.35%) ⬆️

... and 5 files with indirect coverage changes

@utpilla utpilla merged commit 91eb914 into open-telemetry:main Aug 17, 2023
@TimothyMothra TimothyMothra deleted the 4484_refactorHelper branch August 17, 2023 22:15
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