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

chore: remove explicit parent option #1612

Merged

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Oct 21, 2020

Fixes #1611

@codecov
Copy link

codecov bot commented Oct 21, 2020

Codecov Report

Merging #1612 into master will decrease coverage by 0.02%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##           master    #1612      +/-   ##
==========================================
- Coverage   91.22%   91.19%   -0.03%     
==========================================
  Files         165      165              
  Lines        5070     5066       -4     
  Branches     1039     1037       -2     
==========================================
- Hits         4625     4620       -5     
- Misses        445      446       +1     
Impacted Files Coverage Δ
packages/opentelemetry-api/src/trace/NoopTracer.ts 96.55% <75.00%> (-3.45%) ⬇️
packages/opentelemetry-api/src/api/global-utils.ts 100.00% <100.00%> (ø)
packages/opentelemetry-plugin-fetch/src/fetch.ts 96.55% <100.00%> (+0.02%) ⬆️
packages/opentelemetry-tracing/src/Tracer.ts 98.18% <100.00%> (-0.16%) ⬇️

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.

@dyladan
Copy link
Member Author

dyladan commented Oct 28, 2020

Shouldn't we bump this master/packages/opentelemetry-api/src/api/global-utils.ts#L68 to ensure it doesnt cause a problem ?

done @vmarchaud

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

@LukaszKuciel
Copy link

LukaszKuciel commented Feb 7, 2021

I understand that Opentelemetry has not v1 yet but changes like that which brake current usage of parent context should be a little bit more discussed and at least explained in docs how I should now create child span if I don't have Context class instance that with appropriate methods on it, only traceId and parentId.

I'm using Opentelemetry with Google Cloud Trace and I have varial microservices. Requests are processed with many protocols from HTTP, RPC to async PubSub messaging. I pass my span context as simple JSON that I can use as a universal way of creating child spans. At least I used to use it like that because now I can't and I need to stick with @opentelemetry/0.12.0.

@Flarna
Copy link
Member

Flarna commented Feb 8, 2021

@LukaszKuciel There was quite a discussion there: open-telemetry/opentelemetry-specification#875

Please note the OpenTelemetry Js follows the spec in such topics. There were also a lot discussions about the context concept, what it should be used for,... Just browse/read through the closed issues/PRs in spec repo to get details.

But I agree that we could improve regarding docs.
At least the latest breaking changes are documented in Readme (see https://github.com/open-telemetry/opentelemetry-js#upgrade-guidelines).

@LukaszKuciel
Copy link

@LukaszKuciel There was quite a discussion there: open-telemetry/opentelemetry-specification#875

Please note the OpenTelemetry Js follows the spec in such topics. There were also a lot discussions about the context concept, what it should be used for,... Just browse/read through the closed issues/PRs in spec repo to get details.

But I agree that we could improve regarding docs.
At least the latest breaking changes are documented in Readme (see https://github.com/open-telemetry/opentelemetry-js#upgrade-guidelines).

I spent some time reading throughout issues and PRs, change log, etc.. You're doing great job with whole project I really appreciate it. I'm only afraid based on my experience working with it on a complex event-driven architecture that without documentation it's really really hard and takes a lot of time.
This is a huge problem IMHO when GCP says in Google Trace docs that you should use Open Telemetry as standard instead of their own SDK for this product and then you try but you are groping in the dark.

dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
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.

Spec: No explicit parent Span/SpanReference allowed
7 participants