Additional Support for setting fields #31
Conversation
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
There was a problem hiding this comment.
Great PR. I think those are good additions to the span/event template. However, I think the PercentageOfSpan parameter might need some small changes (see comments).
It would also be nice to use the new parameters somewhere in examples/template/template.js for documentation purposes
| // PercentageOfSpan if set controls where in the span the event is generated. If missing, the event is generated randomly | ||
| PercentageOfSpan float32 `js:"percentageOfSpan"` |
There was a problem hiding this comment.
I see a couple of potential improvements here:
- It might be more consistent if the default behavior always adds the event when
PercentageOfSpanis not set - What do you think about renaming it to
RateorEventRatefor clarity? - Unless I'm missing something, the current implementation seems to use PercentageOfSpan only to calculate the event time (L322) but does not influence whether the event is actually added
| rate float32 | ||
| percentageOfSpan float32 |
There was a problem hiding this comment.
The internalEventTemplate already includes a rate field that controls whether an event is added to the span. I think it makes sense to use it here
|
|
||
| var eventTime time.Time | ||
| if e.percentageOfSpan > 0 { | ||
| eventTime = start.Add(time.Duration(float64(duration) * float64(e.percentageOfSpan))) |
There was a problem hiding this comment.
I might be missing something here, but could you explain the rationale behind e.percentageOfSpan controlling the event timestamp?
2 participants
Allows for setting the span status and the percentage of the span that an event occurs at.