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: allow parent span to be null #569

Merged

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Nov 25, 2019

Which problem is this PR solving?

Short description of the changes

  • Allow parent to be null
  • Remove all instances of parent: tracer.getCurrentSpan() || undefined

@codecov-io
Copy link

codecov-io commented Nov 25, 2019

Codecov Report

Merging #569 into master will decrease coverage by 0.26%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #569      +/-   ##
==========================================
- Coverage   92.49%   92.22%   -0.27%     
==========================================
  Files         157      149       -8     
  Lines        7951     7411     -540     
  Branches      682      638      -44     
==========================================
- Hits         7354     6835     -519     
+ Misses        597      576      -21
Impacted Files Coverage Δ
packages/opentelemetry-plugin-dns/src/dns.ts 97.18% <ø> (-0.04%) ⬇️
packages/opentelemetry-plugin-redis/src/utils.ts 90.9% <ø> (-0.4%) ⬇️
...ugin-postgres/opentelemetry-plugin-pg/src/utils.ts 96.96% <ø> (-0.05%) ⬇️
packages/opentelemetry-plugin-grpc/src/grpc.ts 96.15% <ø> (-0.09%) ⬇️
...ges/opentelemetry-core/src/trace/TracerDelegate.ts 100% <100%> (ø) ⬆️
packages/opentelemetry-tracing/src/BasicTracer.ts 100% <100%> (ø) ⬆️
...telemetry-plugin-document-load/src/documentLoad.ts 98.13% <100%> (ø) ⬆️
...n-postgres/opentelemetry-plugin-pg/test/pg.test.ts 94.29% <100%> (ø) ⬆️
...ges/opentelemetry-tracing/test/BasicTracer.test.ts 100% <100%> (ø) ⬆️
...kages/opentelemetry-metrics/test/mocks/Exporter.ts 66.66% <0%> (-33.34%) ⬇️
... and 14 more

Copy link
Member

@markwolff markwolff left a comment

Choose a reason for hiding this comment

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

LGTM


const entries = this._getEntries();

const rootSpan = this._startSpan(
AttributeNames.DOCUMENT_LOAD,
PTN.FETCH_START,
entries,
{ parent: serverContext }
{ parent: parseTraceParent(metaElement?.content ?? '') }
Copy link
Member

Choose a reason for hiding this comment

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

Looks like optional chaining was finally added in TS3.7... Cool!

Copy link
Member Author

Choose a reason for hiding this comment

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

It only works for node apparently. It breaks the browser test.

@obecny
Copy link
Member

obecny commented Nov 25, 2019

I'm not sure if it isn't too late, but for me it looks like generally replacing undefined with null, which might produce more problems with typing in future. Why can't we have undefined instead of null in general and then handle correctly the optional ? param which then by default is undefined not null. So in fact tracer.getCurrentSpan() should be allowed to return undefined instead of null

@obecny
Copy link
Member

obecny commented Nov 25, 2019

Just to give you even better context why it should remain undefined instead of null consider this
JSON.stringify({a: undefined}) // "{}"
vs
JSON.stringify({a: null}) // "{"a":null}"

@dyladan
Copy link
Member Author

dyladan commented Nov 25, 2019

Just to give you even better context why it should remain undefined instead of null consider this
JSON.stringify({a: undefined}) // {"a":null}
vs
JSON.stringify({a: null}) // {}

The returns are opposite but I get the idea. I think I like null in this case though since it means explicitly "there is no parent" as opposed to "no parent was passed to the function"

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

@OlivierAlbertini OlivierAlbertini added the enhancement New feature or request label Nov 26, 2019
@dyladan
Copy link
Member Author

dyladan commented Nov 26, 2019

@mayurkale22 how do you feel about the null/undefined issue?

Regarding @obecny's links:

  • The TS guidelines you posted are specifically for maintainers of typescript and are not meant to be general best practices, it says so right in the document. That being said, "don't use null" is not incredibly helpful advice without a reason.
  • I've seen the Doug Crockford talk before and his problem with null is not the difference between null and undefined, but the fact that either of them exist. It is a fundamental problem with the language and not an issue with usage. Better solutions are things like Option and Result types offered in rust and other languages which allow the compiler to statically verify you have done existence checks and error handling.

I still think null and undefined have different semantic meaning regardless of whether or not they are a good idea as a language-level concept. That said, I don't really feel strongly about it and am happy to just use undefined everywhere if that's what we want to do.

edit: another way to look at it: null is an empty box, undefined is the lack of a box to have anything in

@obecny
Copy link
Member

obecny commented Nov 26, 2019

@dyladan I know these are for maintainers :) but basically I think we might end up with having null and undefined mixed so we will have finally more things like
if (someVar === undefined || someVar === null) or
const someVar: SomeType | undefined | null; for any optional parameters in interfaces

@obecny
Copy link
Member

obecny commented Nov 26, 2019

And still wonder how will the mixed types propagate between different systems when trying to serialise spans with null values. Will all systems handle that correctly as undefined is removed, whereas null stays

@vmarchaud
Copy link
Member

The need for this null check is basically because the ScopeManager use null as default value (cf here, or here).
I would argue that we know in which case the parent can be null (coming from the scope managers) or undefined (not defined).
I don't remember exactly why we choose to use null but i'm pretty sure it was along the line of what @dyladan said (another way to look at it: null is an empty scope, undefined is the lack of a scope to have anything in).

If its really a problem, we will need to edit the scopes managers to use undefined as a default value

@dyladan
Copy link
Member Author

dyladan commented Nov 27, 2019

@vmarchaud even if that's the case, it is of limited value. You treat it the same way every time. Whether the span is undefined or null, you still don't have a parent to put on your span so you still create a new span with no parent.

@vmarchaud vmarchaud self-requested a review November 27, 2019 16:29
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

@dyladan
Copy link
Member Author

dyladan commented Nov 27, 2019

@mayurkale22 think this is ready for merge

@mayurkale22 mayurkale22 added the Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Nov 27, 2019
@mayurkale22 mayurkale22 merged commit b3332e3 into open-telemetry:master Nov 27, 2019
vmarchaud added a commit to mayurkale22/opentelemetry-node that referenced this pull request Dec 10, 2019
vmarchaud added a commit to mayurkale22/opentelemetry-node that referenced this pull request Dec 10, 2019
mayurkale22 pushed a commit to mayurkale22/opentelemetry-node that referenced this pull request Dec 10, 2019
mayurkale22 added a commit that referenced this pull request Dec 10, 2019
* chore(plugin-mongodb-core): add missing codecov script

* fix(mongodb-plugin): check currentSpan against undefined

* chore(scope-managers): return undefined if no scope is found (following #569)
@Flarna Flarna deleted the allow-null-parent branch December 13, 2019 20:54
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
* docs(instrumentation-graphql): improve README.md

* docs: updated README according to PR review

* fix(graphql): fixed graphql example 

it didn't work since @opentelemetry/api package was missing and update @opentelemetry/instrumentation-express
and 
@opentelemetry/instrumentation-graphql

Co-authored-by: Bartlomiej Obecny <bobecny@gmail.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 Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants