#316 conversion to OTEL Metrics #871
Conversation
a0d2e83 to
6285e87
Compare
|
may i move this PR to release/2.7 branch so we can have preview release quickly? |
|
Sounds good to me. I still need to work on the tags aspect & refine the timing. |
totally fine! please take your time, no worries |
6285e87 to
1c662ea
Compare
1c662ea to
a568cfe
Compare
There was a problem hiding this comment.
This implementation is problematic. It ties metrics to tracing. That is, if someone hasn't enabled NATS tracing, but still wants metrics, this implementation won't work.
This mistake has been made due to a misreading of the specs, which recommend setting activity durations for operation duration metrics WHEN an activity/span accompanies the operation for which the metric is being emitted — the specification states the following:
When this metric is reported alongside a messaging span, the metric value SHOULD be the same as the corresponding span duration.
(emphasis mine)
This current implementation, however, has made metrics depend on tracing, which is wrong. It means that the following setup in application code will result in no metrics being reported — violating an obvious user expectation:
builder.Services.AddOpenTelemetry()
.WithTracing(tracerBuilder =>
{
tracerBuilder
.AddAspNetCoreInstrumentation()
.AddHttpClientInstrumentation()
// .AddSource("NATS.Net") // NOTE: you would have to have this for *metrics* to be reported, which makes no sense.
})
.WithMetrics(metricsBuilder =>
{
metricsBuilder.AddMeter("NATS.Net") // NOTE: The metrics are clearly being subscribed to.
})Instead, the metrics implementation should anticipate activities being null and be independently implemented; but optionally use activity durations when the accompanying activity is non-null.
P.S. If you want examples, here's the de facto standard Kafka instrumentation library — it's careful NOT to tie metrics to tracing, as I said: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/03c577940d9b3eb223e21af98ac8eeb692e8e5c9/src/OpenTelemetry.Instrumentation.ConfluentKafka/InstrumentedProducer.cs#L207-L212
|
First off, the implementation is not complete hence why it is in draft status. As such it is unfair to say it is problemtic to use when it is clearly not ready to review especially as a number of aspects are still missing which means that The issues which you pre-emptively foreshadowed would have all been addressed in due course. However to make the review easier when it is ready I will slightly adapt my plans and draw more inspiration from the kafka example. |
d1c1501 to
07a1eb6
Compare
| } | ||
| catch | ||
| var size = payloadBuffer.Length + (headersBuffer?.Length ?? 0); | ||
| if (_connection.ServerInfo is { } info && size > info.MaxPayload) |
There was a problem hiding this comment.
Should just use the size from props.
| @@ -240,19 +231,15 @@ internal async Task InitializeInboxSubscriptionAsync(CancellationToken cancellat | |||
| { | |||
| if (Interlocked.CompareExchange(ref _inboxSub, _inboxSubSentinel, _inboxSubSentinel) == _inboxSubSentinel) | |||
| { | |||
| var inboxSubject = $"{_inboxPrefix}.*"; | |||
| var inboxSubject = new NatsSubscriptionProps($"{_inboxPrefix}.*", _connection.InboxPrefix); | |||
There was a problem hiding this comment.
Check why connection is being used
| @@ -41,8 +42,9 @@ public SubscriptionManager(NatsConnection connection, string inboxPrefix) | |||
| _cleanupInterval = _connection.Opts.SubscriptionCleanUpInterval; | |||
| _timer = Task.Run(CleanupAsync); | |||
| InboxSubBuilder = new InboxSubBuilder(connection.Opts.LoggerFactory.CreateLogger<InboxSubBuilder>()); | |||
| _inboxSubSentinel = new InboxSub(InboxSubBuilder, nameof(_inboxSubSentinel), default, connection, this); | |||
| _inboxSubSentinel = new InboxSub(InboxSubBuilder, new NatsSubscriptionProps(nameof(_inboxSubSentinel), _connection.InboxPrefix), default, connection, this); | |||
There was a problem hiding this comment.
Check which inbox prefix is used
|
@mtmk apologies this has ended up becoming alot bigger than I had hoped especially as I worked through reviewing what was happening and seeing replicated functionality. Are you happy with this PR also including the refactoring? Or would you prefer it be split? As I am nearly done with just a couple of things to look into, when you can could can you start taking a look at it & let me know if you have any questions/feedback. |
thanks for your hard work @thompson-tomo. i'm hoping to have a look this week. it is probably a good idea to split refactoring in general. again in general couple of things we need to make sur 8000 e and keep an eye on: refactoring may have unintentional behavior changes, sometimes it might be desirable to put them behind a feature flag if applicable. another thing is that we have to make sure performance is not affected negatively. |
|
No worries @mtmk I will go about splitting this PR up by making smaller PR's which as they are merged will bring down this PR diff until it is more focused on otel. First of which will be the properties being passed as a record. |
07a1eb6 to
6228cd3
Compare
Signed-off-by: James Thompson <thompson.tomo@outlook.com>
Signed-off-by: James Thompson <thompson.tomo@outlook.com>
Signed-off-by: James Thompson <thompson.tomo@outlook.com>
Signed-off-by: James Thompson <thompson.tomo@outlook.com>
6ea40f6 to
9e8188e
Compare
Signed-off-by: James Thompson <thompson.tomo@outlook.com>
9e8188e to
81f2e50
Compare
This PR focusses on improving the open telemetry integration by introducing support for metrics and refinement of the spans
The duration of the following operations are captured as both spans & duration metrics
Additional metrics include:
Prequiste work
Closes #316