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

Include tags and status in TraceInfo and SegmentInfo #377

Closed
dpsoft opened this Issue Jul 27, 2016 · 7 comments

Comments

Projects
None yet
2 participants
@dpsoft
Contributor

dpsoft commented Jul 27, 2016

No description provided.

@dpsoft dpsoft changed the title from Include Tags in TraceInfo to Include Tag and Status in TraceInfo and SegmentInfo Jul 27, 2016

@dpsoft dpsoft changed the title from Include Tag and Status in TraceInfo and SegmentInfo to Include tags and status in TraceInfo and SegmentInfo Jul 27, 2016

@dpsoft dpsoft closed this in 1873213 Jul 27, 2016

@bfil

This comment has been minimized.

Contributor

bfil commented Aug 11, 2016

@dpsoft I've just tried to add tags to my traces (which are sent to InfluxDB), but I've noticed that only counters and segments tags are working.

I think this change will allow the traces to have tags, so they will be sent on the following line:
https://github.com/kamon-io/Kamon/blob/master/kamon-influxdb/src/main/scala/kamon/influxdb/BatchInfluxDBMetricsPacker.scala#L37

Is that the case? Because at the moment I can see entity.tags coming out on the TickMetricSnapshot as an empty Map, which seems broken, because my trace has tags:

val traceContext = Kamon.tracer.newContext("test", None, Map("tag" -> "value"))
traceContext.finish()

Are you getting this released anytime soon?

@bfil

This comment has been minimized.

Contributor

bfil commented Aug 11, 2016

Ok, I've tested this by building it locally and it fixed the issue with missing tags on the main traces, would be great to get this patch released!

@bfil

This comment has been minimized.

Contributor

bfil commented Aug 12, 2016

@dpsoft I've got another couple more questions actually:

Why have you removed the addTags/removeTags methods? Looked useful to me: 1873213#diff-8cb9f66838d35f02814bdf60bedc6813L113

And is there any reasons why tags cannot be accessed on a TraceContext, only added/removed? Sometimes it might be useful to extract the current tags from the current trace context, the same way you can access the trace name and it's allowed to be renamed. Hope this makes sense.

@dpsoft

This comment has been minimized.

Contributor

dpsoft commented Aug 12, 2016

Hey bruno,

we will publish new release soon.... in relation to addTags/removeTags
methods you can use addTag/removeTag ;), are not usefull for you use case?
also the posibility of accesing to the tags in the TraceContext is a good
idea. #379

Cheers,
Diego

On Fri, Aug 12, 2016 at 7:04 AM, Bruno Filippone notifications@github.com
wrote:

@dpsoft https://github.com/dpsoft I've got another couple of questions
actually:

Why have you removed the addTags/removeTags methods? Looked useful to me:
1873213#diff-8cb9f66838d35f02814bdf60bedc6813L113
1873213#diff-8cb9f66838d35f02814bdf60bedc6813L113

And is there any reasons why tags cannot be accessed on a TraceContext,
only added/removed? Sometimes it might be useful to extract the current
tags from the current trace context.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#377 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACctZS0b8H5Tviqq6qIhHlFFKtGUTYG8ks5qfEU4gaJpZM4JWaHN
.

@bfil

This comment has been minimized.

Contributor

bfil commented Aug 12, 2016

@dpsoft

Sounds great, the only reason behind the addTags/removeTags method is that usually in code I have a map of tags, and I end up having to deconstruct it in order to use the addTag/removeTag methods, would be nice to have both API-wise I guess?

If you would accept a PR for the ability to access the tags in TraceContext I can work on it :)

@dpsoft

This comment has been minimized.

Contributor

dpsoft commented Aug 12, 2016

Please feel free to make a PR.

On Fri, Aug 12, 2016 at 10:24 AM, Bruno Filippone notifications@github.com
wrote:

@dpsoft https://github.com/dpsoft

Sounds great, the only reason behind the addTags/removeTags method is
that usually in code I have a map of tags, and I end up having to
deconstruct it in order to use the addTag/removeTag methods, would be
nice to have both API-wise I guess?

If you would accept a PR for the ability to access the tags in
TraceContext I can work on it :)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#377 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACctZV2yuTCJ0Abl3MSg4MqxiIVgkr2Dks5qfHP3gaJpZM4JWaHN
.

@bfil

This comment has been minimized.

Contributor

bfil commented Aug 12, 2016

@dpsoft here it is: #386

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment