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

export resource to exporters #843

Merged
merged 9 commits into from
Mar 12, 2020

Conversation

mayurkale22
Copy link
Member

@mayurkale22 mayurkale22 commented Mar 5, 2020

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

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
46 tasks
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

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.

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

@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.

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

@@ -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.

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
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
…tion (open-telemetry#843)

Co-authored-by: Nathaniel Ruiz Nowell <enruizno@uwaterloo.ca>
Co-authored-by: Rauno Viskus <Rauno56@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants