Add span_kind to the honeycomb exporter #474
Conversation
|
@lizthegrey can you review this? |
| @@ -177,6 +177,7 @@ func (e *honeycombExporter) pushTraceData(ctx context.Context, td consumerdata.T | |||
| e.sendMessageEvents(td, span, traceLevelFields) | |||
| e.sendSpanLinks(span) | |||
|
|
|||
| ev.AddField("span_kind", span.Kind.String()) | |||
There was a problem hiding this comment.
we typically use meta.span_type in our taxonomy, and lowercase. see https://github.com/honeycombio/beeline-go/blob/76ad42c601cfe475c54a3c73b8ef84c2f4aabbce/trace/trace.go#L457 (cc @maplebed @paulosman)
There was a problem hiding this comment.
meta.span_type is currently used for span_event and link per the metadata kind docs https://docs.honeycomb.io/working-with-your-data/managing-your-data/definitions/#metadata-kind
If sending values other than link/span_event is alright with you, I can change it
There was a problem hiding this comment.
We need a bit of time to think through this API/schema design, is it okay with you if we come back to you in a week with a decision? Regardless, please go ahead and lowercase the constants as we're pretty sure on that.
There was a problem hiding this comment.
Yeah no rush on my end
There was a problem hiding this comment.
Separate action item: I think the exporter needs to be updated to the new internal data format, which will give it access to the other span kinds (producer, consumer, internal)
Codecov Report
@@ Coverage Diff @@
## master #474 +/- ##
=======================================
Coverage 88.86% 88.86%
=======================================
Files 251 251
Lines 11979 11979
=======================================
Hits 10645 10645
Misses 990 990
Partials 344 344
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
|
@lizthegrey do I understand correctly to hold on this PR until next week? |
Affirm, please hold until we can figure out the right schema on our side. Thanks! |
|
Revoking my approval until @lizthegrey confirms the solution. |
bogdandrutu
dismissed
their stale review
Revoking my approval until @lizthegrey confirms the solution.
* Correct README example for tail sampling Signed-off-by: Annanay <annanayagarwal@gmail.com> * Use standard yaml list syntax Signed-off-by: Annanay <annanayagarwal@gmail.com>
|
@lizthegrey ping |
|
ping @paulosman and @MikeGoldsmith |
|
@paulosman and @MikeGoldsmith do you plan to continue working on this? |
|
Ah, this got lost in our backlog. We'll look into it ASAP. |
|
@chris-smith-zocdoc We've finished our internal work and we've settled on Sorry for the delay, there was some additional work we needed to complete beforehand. |
|
Hey @chris-smith-zocdoc, I've re-reviewed your PR and please can you make the following changes:
Thanks! |
|
Please also rebase |
|
@chris-smith-zocdoc If you don't have time to make the changes, I can do them and the rebase. I won't be able to use this PR, but I can base a branch on your original branch so your commits are not lost. Thanks |
chris-smith-zocdoc
force-pushed
the
cs_span_kind
branch
from
8c65946
to
0cd707d
Compare
Replace span_type with annotation_type
|
@MikeGoldsmith rebased and made changes |
|
Thanks @chris-smith-zocdoc. We're happy with the changes above but need to finalise a couple of things on our side. Please do not merge this and I'll update when we're ready. |
|
@MikeGoldsmith please mark this with "required changes" so we don't by mistake merge this |
|
Thanks @bogdandrutu - after some internal comms we're happy for this to merged as there won't be a negative effect if this were merged / released before our internal things are finished up. Happy for this to be merged now 😄 |
* wip: observers * wip: float observers * fix copy pasta * wip: rework observers in sdk * small fix in global meter * wip: aggregators and selectors * wip: monotonicity option for observers * some refactor * wip: docs needs more package docs (especially for api/metric and sdk/metric) * fix ci * Fix copy-pasta in docs Co-Authored-By: Mauricio Vásquez <mauricio@kinvolk.io> * recycle unused recorders in observers if a recorder for a labelset is unused for a second collection cycle in a row, drop it * unregister * thread-safe set callback * Fix docs * Revert "wip: aggregators and selectors" This reverts commit 37b7d05aed5dc90f6d5593325b6eb77494e21736. * update selector * tests * Rework number equality Compare concrete numbers, so we can get actual numbers in the error message when they are not equal, not some uint64 representation. This also uses InDelta for comparing floats. * Ensure that Observers are registered in the same order * Run observers in fixed order So the tests can be reproducible - iterating a map made the order of measurements random. * Ensure the proper alignment of the delegates This wasn't checked at all. After adding the checks, the test-386 failed. * Small tweaks to the global meter test * Ensure proper alignment of the callback pointer test-386 was complaining about it * update docs * update a TODO * address review issues * drop SetCallback Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io> Co-authored-by: Rahul Patel <rghetia@yahoo.com>
All instrumentations packages have almost exactly same setup.py files. This commit adds a python script that generates it from a source template. This dramatically reduces the time and effort required to update all instrumentation setup.py files, and also reduces chances of making manual mistakes.
@paulosman
Adds the span kind to the event sent to Honeycomb, allowing for queries by the type of span (client / server)
Let me know if you thing I should rename the field or lowercase the values.