WIP: Move direct reply handling to be in subscription manager#878
Draft
thompson-tomo wants to merge 6 commits intonats-io:mainfrom
Draft
WIP: Move direct reply handling to be in subscription manager#878thompson-tomo wants to merge 6 commits intonats-io:mainfrom
thompson-tomo wants to merge 6 commits intonats-io:mainfrom
Conversation
Signed-off-by: James Thompson <thompson.tomo@outlook.com>
* nats-io#848 Tweak dependencies Signed-off-by: James Thompson <thompson.tomo@outlook.com> * Update NATS.Client.Core.csproj Signed-off-by: James Thompson <thompson.tomo@outlook.com> * Update NATS.Client.Core.csproj Signed-off-by: James Thompson <thompson.tomo@outlook.com> * Make STJ explicit dependency for JetStream Signed-off-by: James Thompson <thompson.tomo@outlook.com> * Update NATS.Client.JetStream.csproj Signed-off-by: James Thompson <thompson.tomo@outlook.com> * Force sdk to 8.0.0 Signed-off-by: James Thompson <thompson.tomo@outlook.com> * Add newer stj for net 6 Signed-off-by: James Thompson <thompson.tomo@outlook.com> --------- Signed-off-by: James Thompson <thompson.tomo@outlook.com>
thompson-tomo
changed the title
thompson-tomo
force-pushed
the
Task/DemoteReplyFactory
branch
2 times, most recently
from
6813636 to
d22fe57
Compare
mtmk
reviewed
Jun 4, 2025
| if (_bySid.TryGetValue(sid, out var sidMetadata)) | ||
| { | ||
| // this could come from sidMetadata in the future once records are available | ||
| if (_connection.Opts.RequestReplyMode == NatsRequestReplyMode.Direct) |
Member
There was a problem hiding this comment.
i think this should be outside the _bySid check and the lock, no? whole ide of this is to side step the subscription manager internals completely to increase request-reply efficiency.
Contributor
Author
There was a problem hiding this comment.
Yes and no, part of the reason for the change was to get access to the metadata based on subscription id. That metadata can then be used as well as associated with the span etc.
Member
There was a problem hiding this comment.
fair enough. keep in mind we also must make sure the impact to apps not using otel must be bare minimum like a simple bool check
Contributor
Author
There was a problem hiding this comment.
Noted, the usage for the metadata is not limited to OTEL but general usage as well.
…ats-io#859) * Add support for `Filter` and `Enrich` for OpenTelemetry activities * Make `internal` methods in `internal Telemetry` `public` * Fix package versions and whatnot * Remove `TracerProviderBuilderExtensions` * Include `Deserialize` in the receive activity * Revert back accidental change * Add `ParentContext` to `NatsInstrumentationContext` * Make `GetActivityContext` public to provide the ability to get context activity context * Make preprocessor directive more accurate * Revert .csproj formatting * Move public artifacts out of the `Internal` namespace/folder * Fix build script --------- Co-authored-by: Ziya Suzen <ziya@suzen.net>
thompson-tomo
force-pushed
the
Task/DemoteReplyFactory
branch
from
d22fe57 to
bd27cae
Compare
Signed-off-by: James Thompson <thompson.tomo@outlook.com>
thompson-tomo
force-pushed
the
Task/DemoteReplyFactory
branch
from
bd27cae to
1558878
Compare
Member
|
release/2.7 branch is obsolete now. need to move this change to base main. |
mtmk
added
the
obsolete-merge-base
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This does some basic refactoring to move the reply task handler to subscription manager so that it can also leverage subscription metadata which is needed for OTEL #871