Skip to content
This repository has been archived by the owner on Nov 10, 2022. It is now read-only.

feat: make context.active() the default value for certain context functions #30

Closed
wants to merge 2 commits into from

Conversation

naseemkullah
Copy link
Member

@naseemkullah naseemkullah commented Apr 2, 2021

This is actually discussed there #27 for another reason though

functions where I'm adding a default context as context.active(), based on searching the otel-js repo for their invocations:

  • getSpan()
  • setSpan()
  • suppressInstrumentation()

💥 breaks setSpan() as it reverses the order of params.

This PR also serves a discussion. Is this a better default or not?

Pros: it will reduce boilerplate code, helper functions aside/when working with lowest level api

Cons: ?

@codecov
Copy link

codecov bot commented Apr 2, 2021

Codecov Report

Merging #30 (9ae85c2) into main (504041a) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 9ae85c2 differs from pull request most recent head db90004. Consider uploading reports for the commit db90004 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main      #30      +/-   ##
==========================================
+ Coverage   94.64%   94.65%   +0.01%     
==========================================
  Files          39       39              
  Lines         504      505       +1     
  Branches       81       84       +3     
==========================================
+ Hits          477      478       +1     
  Misses         27       27              
Impacted Files Coverage Δ
src/context/context.ts 92.68% <100.00%> (+0.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 504041a...db90004. Read the comment docs.

@naseemkullah naseemkullah marked this pull request as draft April 2, 2021 13:23
@naseemkullah naseemkullah reopened this Apr 2, 2021
@naseemkullah naseemkullah force-pushed the default-ctx branch 2 times, most recently from 84ec2be to ccc3599 Compare April 2, 2021 15:27
@naseemkullah naseemkullah changed the title feat: make context.active() the default value for context functions feat: make context.active() the default value for certain context functions Apr 2, 2021
@naseemkullah naseemkullah marked this pull request as ready for review April 2, 2021 15:30
src/context/context.ts Outdated Show resolved Hide resolved
breaks setSpan as it reverses the order of params

Signed-off-by: naseemkullah <[email protected]>
Signed-off-by: naseemkullah <[email protected]>
@dyladan
Copy link
Member

dyladan commented Apr 5, 2021

This PR also serves a discussion. Is this a better default or not?

In the past we have been moving in the opposite direction. For low-level API primitives we have typically considered it to be better to have more explicit parameters.

/cc @Flarna because I know you've expressed opinions in the past that you believe we already have too many cases where context has a default value of active()

@Flarna
Copy link
Member

Flarna commented Apr 5, 2021

I removed the context.active() as default for propagation.extract() and propagation.inject() in open-telemetry/opentelemetry-js#1734
As mentioned there at least in extract using context.active() (which may contain a span reference) is most likely what noone wants.

The helper functions here are somewhat different as they are used more frequently at places where context.active() is what someone wants - similare as in tracer.startSpan().

I'm not a big fan of using references to globals in default arguments as it makes it terrible hard to consistent use non global ContexManger, TracerProvider,...

I'm also not sure if a breaking change is still acceptable in current stage (see #15 (comment)).

@naseemkullah
Copy link
Member Author

Thanks for the detailed explanation! Closing

@naseemkullah naseemkullah deleted the default-ctx branch April 5, 2021 22:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants