Trace sampling #753
Trace sampling #753
Conversation
| // Custom sampler may not exist or may return null, in which case we fallback | ||
| // to the static sample rate. | ||
| _options.TracesSampler?.Invoke(samplingContext) |
There was a problem hiding this comment.
This is different from Java. I decided to allow the dynamic sampler to return null to fallback to the configured sample rate. I think it would be very useful, in case when you only want special sampling rules for a subset of transactions and default rules for everything else.
There was a problem hiding this comment.
It makes sense to me and I don't have a problem with that.
Maybe @marandaneto has opinions though.
Please make a note on the spec then that this is an alternative approach.
There was a problem hiding this comment.
that sounds a good use case, lets talk about this today in the meeting
There was a problem hiding this comment.
we'll make this on Java too
| /// </summary> | ||
| string? Description { get; set; } | ||
| // 'new' because it adds a setter. | ||
| new string? Description { get; set; } |
There was a problem hiding this comment.
yeah, if the reference held is of a base type this new won't show up. And the wrong getter will run. But hey, not the worst thing we got going on to add this API right?
There was a problem hiding this comment.
The getter will run correctly (since both interfaces still map to the same member on the class). However, if you implement explicitly then yeah you can end up with two different properties:
ISpanContext.Description {get;set;}
ISpan.Description {get;set;}
| @@ -33,17 +33,17 @@ public class Span : ISpan, IJsonSerializable | |||
| /// <inheritdoc /> | |||
| public DateTimeOffset? EndTimestamp { get; private set; } | |||
|
|
|||
| /// <inheritdoc /> | |||
| /// <inheritdoc cref="ISpan.Operation" /> | |||
There was a problem hiding this comment.
Side-effect of having new
| /// <summary> | ||
| /// Span metadata used for sampling. | ||
| /// </summary> | ||
| public class SpanContext : ISpanContext |
There was a problem hiding this comment.
I still don't know why we need SpanContext here though. Both guidelines and Java has it, but it's not used in sampling.
|
@bruno-garcia are you happy with the API? I've commented on some places of interest. |
| // Custom sampler may not exist or may return null, in which case we fallback | ||
| // to the static sample rate. | ||
| _options.TracesSampler?.Invoke(samplingContext) |
There was a problem hiding this comment.
It makes sense to me and I don't have a problem with that.
Maybe @marandaneto has opinions though.
Please make a note on the spec then that this is an alternative approach.
| /// </summary> | ||
| string? Description { get; set; } | ||
| // 'new' because it adds a setter. | ||
| new string? Description { get; set; } |
There was a problem hiding this comment.
yeah, if the reference held is of a base type this new won't show up. And the wrong getter will run. But hey, not the worst thing we got going on to add this API right?
not sure, i just merged origin |
|
@bruno-garcia why is codecov not showing up? :( |
|
@bruno-garcia can you retry appveyor build? Don't know why it broke but I can't merge it because all checks need to pass. |
Related: #663