-
Notifications
You must be signed in to change notification settings - Fork 47
feat: unify signatures of with
and bind
#78
Conversation
src/api/context.ts
Outdated
* @param context context to bind to the event emitter or function. Defaults to the currently active context | ||
* @param target function or event emitter to bind | ||
*/ | ||
public bind<T>(context: Context = this.active(), target: T): T { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove the defaulting to this.active()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be for that change as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would push setting the default to instrumentations which i don't personally mind too.
src/api/context.ts
Outdated
@@ -78,6 +78,7 @@ export class ContextAPI { | |||
|
|||
/** | |||
* Bind a context to a target function or event emitter | |||
* @deprecated in 0.x, will be removed in 1.x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we still have time for breaking changes so i would be in favor of removing the old function in the same PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be less work for me - even better!
src/api/context.ts
Outdated
* @param context context to bind to the event emitter or function. Defaults to the currently active context | ||
* @param target function or event emitter to bind | ||
*/ | ||
public bind<T>(context: Context = this.active(), target: T): T { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would push setting the default to instrumentations which i don't personally mind too.
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
As per apt comments above, I removed any notion of the old method and made bind short circuit if context is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
I'm fine with moving forward with this so i'm requesting changes in this direction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
We are pre-1.x - when's the better time to make breaking changes? |
I would not add this here. Binding this/arguments is an existing JS feature for functions. I don't think we should mix two different concerns in one function. |
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 would not do that. Since we're at 0.x there is no reason to add that additional burden.
No. That's not the purpose of |
I agree, i think since this API is only used by few instrumentations its fine to change it now though |
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:
Thanks to everyone for chiming in! |
I don't see any benefit in writing
instead of
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
2 params (preferred)
or
3 params (preferred)
or
As extra
which I personally hate, and I think we should allow even for this |
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 |
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). In the
Performance is obviously a problem,
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.
Obviously spans is not the only thing we store in the context so i will take this as a suggestion to add 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:
|
I agree with @vmarchaud - explicit is better than implicit(we can always add defaults and utilities). |
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
instead of
And as last: instead of creating withSpan, we could have simply this
|
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
|
Discussion of |
@open-telemetry/technical-committee can you please weigh in on this issue? The maintainers have not been able to come to a unanimous agreement. |
Current situation
The first option has already risen confusions from the users for example: After the proposal from this PR it will look like this, no more optional params will be possible
What I propose is to be able to do this
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 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 @tedsuo ^^ @carlosalberto ^^ |
(1) is out the scope of this PR, lets keep the discussion centered about |
Leave checking special cases for the actual ContextManager and remove the short-circuit from the API implementation.
8e4bbd1
to
c31b6c2
Compare
We do have this concern embedded in |
Its not really the same. 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 Do you have a usecase in mind for binding arguments or are you just trying to mirror the |
That was my thinking, yes.
The latter. There is no absolute need for "argumented"
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 |
If major version is 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. |
This will be included in |
Just like
Function.prototype.apply
,Function.prototype.call
andFunction.prototype.bind
have the same signature, it would make sense forcontext.with
andcontext.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 ofcontext.with
- probably because of howFunction.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:
@vmarchaud makes that point in a slack thread:
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.