-
Notifications
You must be signed in to change notification settings - Fork 853
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
[chore] consolidate scope/resource creation in transformer #4429
Conversation
Signed-off-by: Bogdan Drutu <[email protected]>
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4429 +/- ##
==========================================
- Coverage 92.19% 90.76% -1.44%
==========================================
Files 336 87 -249
Lines 9511 1906 -7605
Branches 2016 405 -1611
==========================================
- Hits 8769 1730 -7039
+ Misses 742 176 -566 |
return { | ||
name: scope.name, | ||
version: scope.version, | ||
droppedAttributesCount: 0, |
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 this can be removed.
droppedAttributesCount: 0, |
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.
It's actually part of the proto (see https://github.com/open-telemetry/opentelemetry-proto/blob/b5c1a7882180a26bb7794594e8546798ecb68103/opentelemetry/proto/common/v1/common.proto#L80)
I think the tests will need to be adapted to expect this new 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.
Already looks very good, thanks for taking care of this. 🙂
A few comments:
- I think the tests need to be adapted to include the
droppedAttributeCount
in the resource assertions. npm run lint:fix
will address any lint issues that are currently causing the lint workflow fail.- please add an entry in
experimental/CHANGELOG.md
for this PR (this will make the changelog workflow pass).
return { | ||
name: scope.name, | ||
version: scope.version, | ||
droppedAttributesCount: 0, |
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.
It's actually part of the proto (see https://github.com/open-telemetry/opentelemetry-proto/blob/b5c1a7882180a26bb7794594e8546798ecb68103/opentelemetry/proto/common/v1/common.proto#L80)
I think the tests will need to be adapted to expect this new 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.
Formally marking this as Changes requested
(see my previous comments), so that we don't merge this until the comments are addressed (the PR has one approval).
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
Merged with the proposed changes and lint fixes in #4600 |
No description provided.