-
Notifications
You must be signed in to change notification settings - Fork 649
Introduce incoming message support for envelope unwrapping #7422
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
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
883a7a4 to
9f82a75
Compare
9b71ba2 to
a8b908e
Compare
a62cd95 to
a48da28
Compare
| if (exception != null) | ||
| { | ||
| meterTags.Add(new KeyValuePair<string, object?>(MeterTags.ErrorType, exception.GetType().FullName)); | ||
| } | ||
|
|
||
| totalEnvelopeUnwrappingErrors.Add(succeeded ? 0 : 1, meterTags); |
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.
What's the scenario for the exception not null and succeeded true?
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.
Shouldn't be such a scenario, but I didn't want to decide the metric value based on the exception and wanted to make it explicit.
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.
If there is no scenario and the method is called RecordEnvelopeUnwrappingError, I don't see the need for the succeeded argument, and its usage.
Thinking a bit more about it, is there a chance for an unwrapping error with no exception?
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.
Error metrics should be "always" emitted. If there is no error, then emit the metric as 0. This way you can make sure that your dashboards and alerts are configured correctly (because the metric is always there and you don't have a typo in your configuration) and that you can have additional alerts on missing metric data points.
That being said, there is a need to differentiate between "error happened" and "no error" situations.
Thinking a bit more about it, is there a chance for an unwrapping error with no exception?
Sure there is (not with the current implementation, though). .NET guidelines suggest that errors are communicated via exceptions, but there is no way to enforce that via the compiler.
| messageContext.NativeMessageId, messageContext.Headers, messageContext.Extensions, | ||
| messageContext.Body); | ||
|
|
||
| metrics.RecordEnvelopeUnwrappingError(messageContext, envelopeHandler, null, true); |
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.
Why are we calling RecordEnvelopeUnwrappingError here? There is no error, at least not yet.
Looks like we want to record three different metrics:
- Successful unwrappings, before line 40
- Failed unwrappings due to unwrapping errors, at line 51, as we already do
- Skipped unwrappers, at line 42
Additionally:
- Do we want to count the number of messages going through as regular NServiceBus messages? (line 74/75)
- Do we want/need to report metrics about how long unwrapping took?
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.
Why are we calling RecordEnvelopeUnwrappingError here? There is no error, at least not yet.
There is no error and we want to explicitly indicate that. As mentioned above, the metric should "always" be emitted.
Looks like we want to record three different metrics:
This only increases the observability cost and makes the configuration more cumbersome. Success vs failure should be (and currently is) captured with one metric.
Skipped unwrappers, at line 42
The unwrapper should emit the metric in that case as it knows the skipping reason (and it indeed does that now). Not sure if we want to have another "generic" metric for that.
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.
Would it be more clear if there was an RecordEnvelopeUnwrappingSuccess or simlar? (to avoid passing null as the exception, I also reacted to calling "error" when it actually succeeded)
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.
Sure, I can rename. The reasoning behind the name is to record the EnvelopeUnwrappingError metric. Given that convention, RecordEnvelopeUnwrappingSuccess would record a different metric. I personally would find that misleading.
| if (exception != null) | ||
| { | ||
| meterTags.Add(new KeyValuePair<string, object?>(MeterTags.ErrorType, exception.GetType().FullName)); | ||
| } | ||
|
|
||
| totalEnvelopeUnwrappingErrors.Add(succeeded ? 0 : 1, meterTags); |
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.
If there is no scenario and the method is called RecordEnvelopeUnwrappingError, I don't see the need for the succeeded argument, and its usage.
Thinking a bit more about it, is there a chance for an unwrapping error with no exception?
src/NServiceBus.AcceptanceTests/Core/OpenTelemetry/Metrics/When_envelope_handler_succeeds.cs
Outdated
Show resolved
Hide resolved
# Conflicts: # src/NServiceBus.Core/Receiving/ReceiveComponent.cs
|
Raised #7583 with a proposal to use the new syntax for done conditions |
src/NServiceBus.Core/Pipeline/Incoming/IncomingPipelineMetrics.cs
Outdated
Show resolved
Hide resolved
* Convert to new style of acceptance tests * Fix warning * Field --------- Co-authored-by: Daniel Marbach <daniel.marbach@openplace.net>
|
Could potentially multiple envelopes be nested? As in a sort of envelope inception? In general there should only be 1 envelope but suddenly I got enterprise integration nostalgia where I was at a project that did that. Have a custom format wrapped in XML and then again in a SOAP envelope. |
It could. To our knowledge, it's neither known nor supported by any spec. |
Current flow stops when an unwrapper was successful in the current Second, this seems to be the only way to have multiple unwrappers applied. For example if a use would want to have separate unwrappers for (de)compression and/or (un)encryption. |
If that becomes a real requirement we could simply make our unwrappers public and then a user could decorate an unwrapper given they already said you can remove or add unwrappers, no? every time I write wrapper I silently whisper an eminem song |
This is not fully correct. It's not just unwrapper, it's envelope unwrapper. We are dealing with envelopes only. Encryption, compression, obfuscation, and other stuff - these are not envelopes. It's just like in the OSI/ISO networking model - both Layers 2 and 3 have their own envelopes, but they are not covered by one spec and are not on the same level. One can't just add encryption on Layer 3 and expect Layer 2 to deal with that. Our envelope unwrappers are meant to support some specification. If the specification supports recursive unwrapping - the unwrapper will need to handle that. When you say Jokingly, one can obviously come up with another incarnation of decorator pattern to which I can only reply with https://xkcd.com/927/ |
…e-methods Use a multiple methods to report the envelope unwrapping metric counter
* Adjust envelope handler signature to allow proper pooling provided by an ArrayPoolBufferWriter * Better grouping of incoming message information vs output * Move writer * Pooling only needs to happen when an unwrapper actually does something * Remove the WrittenCount check because it is the responsibility of the unwrapper to write the body to the writer * Always clear the buffer before trying the following unwrapper --------- Co-authored-by: Daniel Marbach <danielmarbach@users.noreply.github.com> Co-authored-by: Mauro Servienti <mauro.servienti@gmail.com>
This pull request introduces a new extensibility point in NServiceBus for envelope unwrapping, allowing the registration of envelope handlers for incoming messages, such as CloudEvents. It adds the
IEnvelopeHandlerinterface and supporting infrastructure, integrates envelope handler registration into endpoint and feature configuration, and introduces OpenTelemetry metrics for envelope unwrapping successes and failures. Comprehensive tests are provided for both the envelope unwrapping logic and the new metrics.This new extensibility point will first be used to enable cloudevents with the Azure Service Bus and Amazon SQS transports:
Samples for will be added to the documentation with:
Envelope Handler Extensibility Infrastructure:
IEnvelopeHandlerinterface, which allows custom logic for unwrapping incoming message envelopes.EnvelopeComponentandEnvelopeUnwrapperclasses to manage and invoke registered envelope handlers. TheEnvelopeComponentcollects handler factories and creates the unwrapper at runtime.EnvelopeConfigExtensionsfor registering envelope handlers in endpoint and feature configuration via the newAddEnvelopeHandler<THandler>()extension method.Integration into Endpoint Lifecycle:
Metrics and Observability:
nservicebus.envelope.unwrapping_error(Counter), including new tags such asnservicebus.envelope.unwrapper_typeanderror.typefor observability of envelope unwrapping outcomes.Testing:
EnvelopeUnwrapperTests.These changes collectively enable advanced envelope customization and improved monitoring for message processing in NServiceBus.