-
Notifications
You must be signed in to change notification settings - Fork 780
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] Cleanup shared code #5622
Conversation
It was migrated to the contrib repository
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5622 +/- ##
==========================================
+ Coverage 83.38% 85.76% +2.38%
==========================================
Files 297 254 -43
Lines 12531 11016 -1515
==========================================
- Hits 10449 9448 -1001
+ Misses 2082 1568 -514
Flags with carried forward coverage won't be shown. Click here to find out more. |
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 with some minor questions (non-blocking)
public const string AttributeUrlFull = "url.full"; // replaces: "http.url" (AttributeHttpUrl) | ||
public const string AttributeUrlPath = "url.path"; // replaces: "http.target" (AttributeHttpTarget) | ||
public const string AttributeUrlQuery = "url.query"; | ||
public const string AttributeUrlScheme = "url.scheme"; // replaces: "http.scheme" (AttributeHttpScheme) |
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.
There was an early effort (#5432) to sync the shared files between opentelemetry-dotnet
and opentelemetry-dotnet-contrib
. We might either need to keep the files in sync or as Reiley recommended move these attributes to actual implementation.
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 think that this repository needs only small subset of full semantic convetion from contrib repository. We can move to actual implementation, but it could be more error prone (two copies of the same string). I would not expect frequent changes in attributes/resources needed in this repository (most of them already should be stable).
Related to open-telemetry/opentelemetry-dotnet-contrib#1806
Changes
Cleanup after
Merge requirement checklist
[ ] AppropriateCHANGELOG.md
files updated for non-trivial changes[ ] Changes in public API reviewed (if applicable)