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.

Sign up for GitHub

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

Jump to bottom

export resource to exporters #843

Merged
merged 9 commits into from
Mar 12, 2020
Merged

export resource to exporters #843

merged 9 commits into from
Mar 12, 2020

Conversation

mayurkale22
Copy link
Member

@mayurkale22 mayurkale22 commented Mar 5, 2020 •
edited
Loading

Would like to get early reviews on this before completing all the work.

/cc @mwear

@dyladan dyladan added this to the Beta milestone Mar 5, 2020
@dyladan dyladan added the enhancement New feature or request label Mar 5, 2020
@codecov-io
Copy link

codecov-io commented Mar 5, 2020 •
edited
Loading

Codecov Report

Merging #843 into master will increase coverage by 1.65%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #843      +/-   ##
==========================================
+ Coverage   92.63%   94.29%   +1.65%     
==========================================
  Files         252      255       +3     
  Lines       11113    11140      +27     
  Branches     1069     1077       +8     
==========================================
+ Hits        10295    10504     +209     
+ Misses        818      636     -182
Impacted Files Coverage Δ
...-exporter-collector/src/platform/node/sendSpans.ts 0.00% <ø> (ø)
...-exporter-stackdriver-trace/test/transform.test.ts 100.00% <ø> (ø)
...emetry-exporter-collector/src/CollectorExporter.ts 91.11% <100.00%> (+0.41%) ⬆️
...ges/opentelemetry-exporter-jaeger/src/transform.ts 100.00% <100.00%> (ø)
...entelemetry-exporter-jaeger/test/transform.test.ts 100.00% <100.00%> (ø)
...emetry-exporter-stackdriver-trace/src/transform.ts 100.00% <100.00%> (ø)
...ges/opentelemetry-exporter-zipkin/src/transform.ts 100.00% <100.00%> (ø)
...entelemetry-exporter-zipkin/test/transform.test.ts 100.00% <100.00%> (ø)
...ges/opentelemetry-tracing/src/NoopSpanProcessor.ts 50.00% <0.00%> (-10.00%) ⬇️
...es/opentelemetry-node/src/instrumentation/utils.ts 90.47% <0.00%> (-9.53%) ⬇️
... and 57 more

@dyladan dyladan mentioned this pull request Mar 6, 2020
Closed
46 tasks
vmarchaud
vmarchaud approved these changes Mar 6, 2020
View reviewed changes
Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

lgtm

mwear
mwear approved these changes Mar 9, 2020
View reviewed changes
Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

This looks good to me @mayurkale22. The other part of this work is here: #846 and we have some duplication that will need to be resolved.

@mayurkale22
Copy link
Member Author

The other part of this work is here: #846 and we have some duplication that will need to be resolved.

Thanks for the #846, I will rebase and resolve the conflicts after your PR is merged.

dyladan
dyladan approved these changes Mar 10, 2020
View reviewed changes
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

there are conflicts but this looks fine to me

@mayurkale22 mayurkale22 self-assigned this Mar 10, 2020
@dyladan
Copy link
Member

dyladan commented Mar 11, 2020

open-telemetry/opentelemetry-specification#508

@mayurkale22
Copy link
Member Author

This looks good to me @mayurkale22. The other part of this work is here: #846 and we have some duplication that will need to be resolved.

Rebased with #846, PTAL.

obecny
obecny suggested changes Mar 11, 2020
View reviewed changes
Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

I think the type mismatch

packages/opentelemetry-exporter-collector/src/platform/node/sendSpans.ts Outdated
@@ -70,7 +73,7 @@ export function sendSpans(
},
attributes: collectorExporter.attributes,
},
// resource: '', not implemented
resource: resource.labels,
Copy link
Member

Choose a reason for hiding this comment

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

I think this is wrong
the Resource type for collector is this:
https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-exporter-collector/src/types.ts#L270

Can you verify that you actually see the resource in collector ?

Copy link
Member

Choose a reason for hiding this comment

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

I think the build would fail if the type mismatched

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you verify that you actually see the resource in collector ?

I am going to run the example and verify the behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried the basic example with Collector + Zpkin Exporter, everything is working file. However, I don't see resource/attributes in zipkin UI because Collector Zipkin is not exporting resource data.

Copy link
Member

Choose a reason for hiding this comment

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

but the interface shows that the resource is not label, why this
resource: resource.labels
instead of
resource: {labels: resource.labels, type: 'some type'} ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@obecny thanks for catching this, I addressed in b7c7dc0. Somehow the compiler didn't catch that.

obecny
obecny approved these changes Mar 12, 2020
View reviewed changes
Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm, thx for fixing the resource

@mayurkale22 mayurkale22 merged commit 4627892 into open-telemetry:master Mar 12, 2020
@mayurkale22 mayurkale22 deleted the export_resource branch March 12, 2020 23:03
@mayurkale22 mayurkale22 mentioned this pull request Mar 17, 2020
Closed
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
@nozik @NathanielRN @rauno56
feat: support baggage propagation in aws lambda custom context extrac…
da792fe 
…tion (open-telemetry#843)

Co-authored-by: Nathaniel Ruiz Nowell <[email protected]>
Co-authored-by: Rauno Viskus <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dyladan dyladan dyladan approved these changes

@OlivierAlbertini OlivierAlbertini OlivierAlbertini approved these changes

@vmarchaud vmarchaud vmarchaud approved these changes

@mwear mwear mwear approved these changes

@obecny obecny obecny approved these changes

@bg451 bg451 Awaiting requested review from bg451

@markwolff markwolff Awaiting requested review from markwolff

@rochdev rochdev Awaiting requested review from rochdev

Assignees

@mayurkale22 mayurkale22

Labels
enhancement New feature or request
Projects
None yet
Milestone
Beta
Development

Successfully merging this pull request may close these issues.
7 participants
@mayurkale22 @codecov-io @dyladan @obecny @mwear @vmarchaud @OlivierAlbertini