feat: unify signatures of with and bind

[rauno56]

Just like Function.prototype.apply, Function.prototype.call and Function.prototype.bind have the same signature, it would make sense for context.with and context.bind to do the same to prevent confusion among the instrumentation developers(I know I have made the mistake a few times).

Having context as the first argument also makes intuitive sense, to me at least, even without considering the signature of context.with - probably because of how Function.prototype.bind works.

One drawback for this is the fact that the function argument to both of those methods is more "stable" - meaning that making the function an optional parameter makes no sense, but context, on the other hand, has an intuitive default: the active context. So:

api.context.bind(undefined, () => {
  // binds to active context.
});

@vmarchaud makes that point in a slack thread:

context.with is used to apply a context within the callback function, it doesn't make sense to use it if you don't want to change the context
whereas context.bind is used to correctly propagate the context even if the method is known to "break" propagation like events emitter, so it makes sense to bind the current context in this case

This PR is a hypothetical one to spawn discussion on the topic, the actual implementation needs, for the time being, do some parameter type checking in context.bind, which can be removed before the release of 1.x.
Unless this change gets significant pushback, I to do the same change in Context Manager's APIs as well and prepare another PR to be merged before 1.0 removing deprecated options.

Implemented the way it is right now, this is BREAKING CHANGE.

[Flarna]

I would remove the defaulting to this.active().

[rauno56]

I'd be for that change as well.

[vmarchaud]

That would push setting the default to instrumentations which i don't personally mind too.

[vmarchaud]

we still have time for breaking changes so i would be in favor of removing the old function in the same PR

[rauno56]

That would be less work for me - even better!

[vmarchaud]

That would push setting the default to instrumentations which i don't personally mind too.

[codecov]

Codecov Report

Merging #78 (3a74ec3) into main (ee57789) will not change coverage.
The diff coverage is 66.66%.

@@           Coverage Diff           @@
##             main      #78   +/-   ##
=======================================
  Coverage   94.83%   94.83%           
=======================================
  Files          42       42           
  Lines         561      561           
  Branches       88       87    -1     
=======================================
  Hits          532      532           
  Misses         29       29           

Impacted Files	Coverage Δ
src/api/context.ts	96.15% <50.00%> (ø)
src/context/NoopContextManager.ts	100.00% <100.00%> (ø)

---

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 ee57789...3a74ec3. Read the comment docs.

[rauno56]

As per apt comments above, I removed any notion of the old method and made bind short circuit if context is undefined.

[vmarchaud]

Both the noop context manager (https://github.com/open-telemetry/opentelemetry-js-api/blob/main/src/context/NoopContextManager.ts#L34), the interface type (https://github.com/open-telemetry/opentelemetry-js-api/blob/main/src/context/types.ts#L68) and context api (https://github.com/open-telemetry/opentelemetry-js-api/blob/main/src/api/context.ts#L85) need to be updated

[vmarchaud]

I'm fine with moving forward with this so i'm requesting changes in this direction

[obecny]

both functions have different number of params already, and this is huge breaking change, you can't align all params anyway with this, I don't really see benefit in this, beside making one more breaking change and you wont be able to align params anyway.

[rauno56]

both functions have different number of params already, and this is huge breaking change, you can't align all params anyway with this, I don't really see benefit in this, beside making one more breaking change and you wont be able to align params anyway.

Do you think it would be worth adding bound arguments to context.bind?
I see how that would align those two methods even better, but compared to having arguments in a different order it's a minor inconvenience for an edge case.

this is huge breaking change

We are pre-1.x - when's the better time to make breaking changes?
I can implement this in a non-breaking way, by supporting both for at least until 1.x + bind is barely used compared to with, so "huge" is I think a bit of an overstatement.

[Flarna]

Do you think it would be worth adding bound arguments to context.bind?

I would not add this here. Binding this/arguments is an existing JS feature for functions.
context.bind() is intended to bind a context on a function or Eventemitter.

I don't think we should mix two different concerns in one function.

[dyladan]

This only changes the context api but the context manager itself is still the other way around. In my opinion that's even more confusing. We should change both or neither but only changing one is definitely not the way to go.

I can implement this in a non-breaking way, by supporting both for at least until 1.x

I would not do that. Since we're at 0.x there is no reason to add that additional burden.

Do you think it would be worth adding bound arguments to context.bind?

No. That's not the purpose of bind.

[vmarchaud]

This only changes the context api but the context manager itself is still the other way around. In my opinion that's even more confusing. We should change both or neither but only changing one is definitely not the way to go.

I agree, i think since this API is only used by few instrumentations its fine to change it now though

[rauno56]

This only changes the context api but the context manager itself is still the other way around. In my opinion that's even more confusing. We should change both or neither but only changing one is definitely not the way to go.

This is not the final PR. It can sometimes be hard to gauge how willing are we to change some things. I wanted to get feedback on the API change before I do any proper work(happy that I did!). The APIs for the context manager and the API here should be the same and I'll work on that.

To conclude:

• Binding arguments should not be added to bind.
• We are leaning towards going forward with the proposed API change of
  1. swapping the order of the arguments and
  2. removing the context defaulting to current.active().
• Even though this is a breaking change, we don't see a pressing reason to go through the hassle of assuring the backwards compatibility for this.
• Context Managers and instrumentations also need a PR to align everything to the same signature.

Thanks to everyone for chiming in!

[obecny]

Do you think it would be worth adding bound arguments to context.bind?
I see how that would align those two methods even better, but compared to having arguments in a different order it's a minor inconvenience for an edge case.

I don't see any benefit in writing

context.bind(context.active(), target)

instead of

context.bind(target)

Moreover I think it will just produce an extra unnecessary code to be always included, instead of intuitively using the api, people will need to copy boiler plate code, or they will be asking how to do this - especially new comers, until they will gain knowledge "how to do the things right" and they will always have to copy paste the the boiler plate code

I would be in favour of allowing the following

1 param

context.bind(target); // use default context = context.active();
context.with(target); // use default context = context.active();

2 params

(preferred)

context.bind(target, someContext);
context.with(target, someContext);

or

context.bind(someContext, target);
context.with(someContext, target);

3 params

(preferred)

// bind not applicable
context.with(target, someContext, args);

or

// bind not applicable
context.with(someContext, target, args);

As extra
For the user convenience if we are able to correctly detect which param is context and which param is target, we could easily allow both orders - personally I would not see a problem with this tbh. But I'm very strong against necessity of including context.active() as something that need to be included always exactly what we have with this

api.context.with(api.setSpan(api.context.active(), span))

which I personally hate, and I think we should allow even for this api.context.with(span, ()=> {}) as a shortcut in 95% of cases.

[dyladan]

Based on @obecny comment above it looks like his proposal is:

with(callback, context = this.active())
bind(target, context = this.active())

I think it's important to unify the implementations of with and bind. Personally, I would have preferred to make context always explicit (no default arguments), but I think @obecny's option is reasonable and it seems like his opinion is stronger than mine.

[vmarchaud]

I think it's important to unify the implementations of with and bind. Personally, I would have preferred to make context always explicit (no default arguments)

I will join daniel opinion here, explicit is much better in this case. Context propagation is really hard to understand and use correctly (because node have an single threaded event loop).
I believe i've spent more than enough working on the async hooks context manager (open-telemetry/opentelemetry-js#1099, open-telemetry/opentelemetry-js#1011, open-telemetry/opentelemetry-js#103) to say that we should avoid having defaults that makes user not understand whats going on.

In the with case:

• having a default with(callback, context = this.active()) makes no sense, with should be used to change the context so i don't see why a default would help.
• If having a default makes no sense, i don't see the gain of swaping both arguments

For the user convenience if we are able to correctly detect which param is context and which param is target, we could easily allow both orders - personally I would not see a problem with this tbh

Performance is obviously a problem, with or bind can be called a lot depending on instrumentation usage, checking everytime the order makes us rely on JIT optimisation to be performant which i think is a strong no-go.

Moreover I think it will just produce an extra unnecessary code to be always included, instead of intuitively using the api, people will need to copy boiler plate code, or they will be asking how to do this - especially new comers, until they will gain knowledge "how to do the things right" and they will always have to copy paste the the boiler plate code

We are talking about less than 5% of users that will use manual context propagation, not everyone have complex enterprise requirement, some just want auto intrumentations to do the work. For me it makes sense that people using manual propagation understand what the context is about and how it works. I agree that this is not documented at all right now but that doesnt mean it can't be in the future.

which I personally hate, and I think we should allow even for this api.context.with(span, ()=> {}) as a shortcut in 95% of cases.

Obviously spans is not the only thing we store in the context so i will take this as a suggestion to add withSpan, which i'm totally fine with on the trace API.

In conclusion, i believe context API methods should be low level and explicit, if users find it too complicated that means we need high level helpers on each signal's API (i.e: withSpan).
What i suggest is:

• Go ahead with this PR, requiring a explicit context for bind as 1 argument.
• Debate if startActiveSpan (which is even simpler than would be withSpan) is simple enough for end users.

[rauno56]

I agree with @vmarchaud - explicit is better than implicit(we can always add defaults and utilities).
I also feel strongly for the arguments to be in the order of (context, fn) and not (fn, context), since the function can be long and hide the context + it's more familiar with the ones on Function.prototype.

[obecny]

As I wrote I'm fine with both orders, I have preferred one, but this is not a blocker that's why I marked this as preferred. What I will not be ok with is that you guys want to force user to create a boiler plate code even though it can be simplified and in js it works like this everywhere. Whatever is decided I think the user should still be able to call this with one param only

bind(target);

instead of

bind(context.active(), target);
// or (preferred)
bind(target, context.active());

And as last:

instead of creating withSpan, we could have simply this

context.with(span, ()=> {
  //
});

[vmarchaud]

instead of creating withSpan, we could have simply this
context.with(span, ()=> {
//
});

This is not possible, the context is not only made for spans, it need to be agnostic.

[obecny]

instead of creating withSpan, we could have simply this
context.with(span, ()=> {
//
});

This is not possible, the context is not only made for spans, it need to be agnostic.

Of course this is possible, here is implementation

  public with<A extends unknown[], F extends (...args: A) => ReturnType<F>>(
    contextOrSpan: Context | Span,
    fn: F,
    thisArg?: ThisParameterType<F>,
    ...args: A
  ): ReturnType<F> {
    let context: Context;
    if (typeof (contextOrSpan as Span).spanContext === 'function') {
      context = setSpan(this.active(), contextOrSpan as Span);
    } else {
      context = contextOrSpan as Context;
    }
    return this._getContextManager().with(context, fn, thisArg, ...args);
  }

[dyladan]

Discussion of with(span..., possible or not, is outside of the scope of this PR. Let's try to keep discussion just to the argument ordering for now.

[dyladan]

@open-telemetry/technical-committee can you please weigh in on this issue? The maintainers have not been able to come to a unanimous agreement.

[obecny]

Current situation
When user wants to use it - it look like this

1. api.context.with(api.setSpan(api.context.active(), span), callback)
2. api.context.bind(target)

The first option has already risen confusions from the users for example:
open-telemetry/opentelemetry-js#1923 (comment)
open-telemetry/opentelemetry-js#2181

After the proposal from this PR it will look like this, no more optional params will be possible

1. no changes
2. api.context.bind(api.context.active(), target)

What I propose is to be able to do this

1. api.context.with(span, callback)
2. api.context.bind(target)

The order of arguments as I mentioned already is not as important as being able to simplify things because user will be still able to write either this
api.context.bind(api.context.active(), target)
or this
api.context.bind(target, api.context.active())

Depends which order of arguments is preferred

The big NO for me is that we will force user to have boiler plate code, instead of simplifying things and making it easier as user will always have to copy api.context.active() whenever you use bind which simply sounds wrongly

@tedsuo ^^ @carlosalberto ^^

[dyladan]

What I propose is to have this

1. api.context.with(span, callback)
2. api.context.bind(target)

(1) is out the scope of this PR, lets keep the discussion centered about api.context.bind here

[dyladan]

@rauno56 following maintainers vote in #86 you can move forward with this PR with a required context argument.

#86

[rauno56]

Do you think it would be worth adding bound arguments to context.bind?

I would not add this here. Binding this/arguments is an existing JS feature for functions.
context.bind() is intended to bind a context on a function or Eventemitter.

I don't think we should mix two different concerns in one function.

We do have this concern embedded in with already...

[dyladan]

Do you think it would be worth adding bound arguments to context.bind?

I would not add this here. Binding this/arguments is an existing JS feature for functions.
context.bind() is intended to bind a context on a function or Eventemitter.
I don't think we should mix two different concerns in one function.

We do have this concern embedded in with already...

Its not really the same. with calls the function, so it makes sense that you can apply arguments. bind doesn't call the function, so the arguments will be provided at call time.

boundFn = context.bind(ctx, fn, arg1, arg2);
boundFn(arg3, arg4); // which arguments get used here? what happened to arg1 and arg2?

// returns the result of calling fn with arg1 and arg2 as arguments
result = context.with(ctx, fn, arg1, arg2);

edit:

hmm. looking at Function.prototype.bind it looks like arg1 and arg2 would be prepended at call time so it would be equivalent to fn(arg1, arg2, arg3, arg4). I still think that is quite nuanced and possibly confusing to new users.

Do you have a usecase in mind for binding arguments or are you just trying to mirror the Function.prototype.bind behavior?

[rauno56]

hmm. looking at Function.prototype.bind it looks like arg1 and arg2 would be prepended at call time so it would be equivalent to fn(arg1, arg2, arg3, arg4). I still think that is quite nuanced and possibly confusing to new users.

That was my thinking, yes.

Do you have a usecase in mind for binding arguments or are you just trying to mirror the Function.prototype.bind behavior?

The latter.
I feel I could wildly imagine up a corner-case situation where it would come in somewhat handy, but compared with even Function.prototype.bind(which I do often use), which is already a relatively lesser-known and -used feature it's even more so. So.. I'm going to save everyone from that. :)

There is no absolute need for "argumented" context.with, because

1. one could use Function.prototype.bind or;
2. alternatively define a wrapper arrow function or;
3. 90% of the time when it's possible just reference the arguments from the outer scope(which is potentially bad for GC optimizations);
   but I think it's extremely convenient.

I find the current solution a practical one, I'd be happy going with!

I'd love to see this being merged and released ASAP so we can quickly test and release SDK changes as well. However, users referencing ^0.18.0 or the like, would immediately get that installed, breaking every instrumentation using context.bind.
Ideas how to mitigate those issues?

[Flarna]

However, users referencing ^0.18.0 or the like, would immediately get that installed, breaking every instrumentation using context.bind.
Ideas how to mitigate those issues?

If major version is 0 the ^ range will not select new minor versions. so ^0.18.0 is the same as 0.18.x.

There are a lot other breaking changes between 0.18.0 and 0.20.0 and there will be other ones after 0.20.0 besides this.

[dyladan]

This will be included in 0.21.x release which will be released with npm @next tag so that users who run npm i @opentelemetry/api won't receive it (that gives them the @latest tag). Core and contrib will then be updated (also using @latest tag). When all compatible versions are released with @next, I will apply the @latest tag.