Namespace prebuild tracing tags #8648
Conversation
easyCZ
force-pushed
the
af/rate-limits
branch
2 times, most recently
from
5abcbe1 to
519dbe9
Compare
easyCZ
force-pushed
the
mp/prebuilds-tags
branch
from
024b4a5 to
ac3c87e
Compare
| span.setTag("prebuild.cloneURL", cloneURL); | ||
| span.setTag("prebuild.commit", commit); | ||
| span.setTag("prebuild.branch", branch); | ||
| span.setTag("prebuild.project", project); |
There was a problem hiding this comment.
Project, for example, is a first class domain property which exists in other features. It feels odd to namespace it with prebuild. This effectively prevents tracing across services and features.
There was a problem hiding this comment.
@easyCZ I agree. But it's hard to get this right if you want to follow the "separate by domain" pattern. Like, in this case that would give:
context.contextURL
context.cloneURL
context.commit
context.branch
project
But:
- our internal shapes are different
- there are times where it's not clear what "the" commit of a request is
- impossible to enforce correctness, and hard to get right because it's not function-local
IMO it's easier to follow the "namespace by span/function" pattern, except for very generic use cases (we do that for Owner/Workspace/Instance in GitpodServerImpl, for instance). Lookup works surprisingly well - you have the code open for that mostly anyways.
BTW, here's a document started by @mads-hartmann on the topic of naming convetions. I bet he has an opinion on this as well 😉
There was a problem hiding this comment.
How to namespace your attributes depends on how you'd like to query for it.
- Find all spans related to a specific entity (e.g. user). Then you'd do something like
COUNT where user.id = "foobar" group by name. A query like that is only possible if we useuser.idfor the user id everywhere. - Filter/group a specific span based on attributes. For this it's a much nicer experience in Honeycomb if the attributes are namespaced using the name of the span. E.g. in this case
startPrebuild.cloneURL. When you write your Honeycomb query you can then doCOUNT where name = startPrebuild, startPrebuild.<autocomplete>and it will show all the attributes that it has ever seen for that span. If we didn't namespace it then you'd have to know attributes might be available on the span, e.g. thatcontext.cloneURLhappens to be a valid attribute for that span☺️
So for this PR I would use the startPrebuild namespace for all the attributes (so rename prebuild to startPrebuild). If you believe the use-case (1) is valid for any of the attributes then just add those as well. A bit of duplication is fine, we pay per event, not by the number of attributes.
If you do add a new top-level namespace please do update the document though!
There was a problem hiding this comment.
From my experience, I'd actually expect both to work. Let me elaborate:
I would indeed expect to be able to find all interactions by a given user, or everything related to a given workspace. Here I'd use the user.id or workspace.id fields.
But I'd also expect to be able to drill deeper and get all prebuild (startPrebuild) traces and do operations on them which aggregate over (or away) the user.id or workspace.id fields. I'd expect to query this with a more specific tag - component: prebuild or something along the lines. This would be yet another field which helps me go deeper. Each span could then also have very custom attributes of course but those would be required to be namespaced as I wouldn't expect these to be cross-cutting, or have semantics outside of a given component: prebuild trace.
I'm happy to follow whichever guidence you provide as I don't yet have enough context but it feels we're preventing ourselves from leveraging some features of distributed tracing.
There was a problem hiding this comment.
I'm happy to follow whichever guidence you provide as I don't yet have enough context but it feels we're preventing ourselves from leveraging some features of distributed tracing.
We are not, it's a trade-off. If you want to get discoverability of what attributes are available on a specific span as you write your query for that specific span, then have to go with (2).
If you want to find all spans for a specific entity go with (1).
If you need both, go with (1) and (2).
In my experience, when you're debugging you're mostly trying to figure out why a specific operation (span) is slow. For that you want to run some aggregates over all of those spans and do some grouping on the attributes to help you drill down. So providing a good experience when writing queries of the form <AGGREGATE> WHERE name = startPrebuild GROUP BY startPrebuild.XXX <CODE COMPLETE> is a worthwhile thing to optimise for.
And as I said, you can just add both startPrebuild.context.id and context.id. A bit of duplication doesn't hurt.
Codecov Report
@@ Coverage Diff @@
## main #8648 +/- ##
=========================================
- Coverage 11.17% 7.34% -3.84%
=========================================
Files 18 32 +14
Lines 993 2234 +1241
=========================================
+ Hits 111 164 +53
- Misses 880 2067 +1187
- Partials 2 3 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Description
Namespace
startPrebuildtags as mentioned in #8568 (comment)Open Questions
contextURL,projectand few others feel like shared domain tags between many RPCs - do we have any unified tagging for domain properties which are wide-reaching? In practice, we want to be able to find relevant traces for any of these properties and for that we need a common tag schema.How to test
Release Notes