Add shared spec for span type/subtype#443
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Trends 🧪 |
I'd say yes. Even if something seems language-specific now, it can be relevant for others in the future. It also forces to think about how to make things generic enough so it may be used by other languages. We'll probably amend this list with other agent-specific types/subtypes.
I'd err on the side of including platform-specific type/subtypes. If it turns out that updating the spec becomes too frequent and tedious, we can think about allowing any subtypes of a type. Overall, I think it's fine to include platform-specific subtypes. It gives a good overview on what we support and helps to avoid subtle differences in the subtypes we use in the future. |
|
Main PR description has been updated to reflect our current aim: document the status quo and make a bit more obvious that further alignment issues/PRs will be handled later. |
basepi
left a comment
There was a problem hiding this comment.
My only nit: I'm unsure that "__used_by" is actually useful. It will be hard to keep up to date and whether an agent is using it yet or not doesn't seem to me to matter much.
I agree with you, it does not matter at all when enforcing the spec, but the purpose is more about documentation & help with alignment, which will come next:
Once we have proper alignment, there won't be any need to keep it, thus it's only temporary. |
astorm
left a comment
There was a problem hiding this comment.
@SylvainJuge Thanks for taking this on.
I did a quick survey of the types and subtypes in the Node.js Agent and I discovered a few discrepancies with what's in this spec PR.
First -- there's two types in the Node.js Agent that aren't present in span_types.json
Type: websocket
Subtypes: send
Type: cache
Subtypes: redis
One is for instrumenting the sending of websocket message via https://www.npmjs.com/package/ws
The other appears to be a place where the Node.js has placed its redis spans under a cache type instead of a db type. I'm unsure of the history here.
Second -- there's three db subtypes from the Node.js Agent that are missing from span_types.json.
Type: db
Subtypes: mssql, memcached, graphql
The Node.js uses the memcached subtype for instrumenting memcached operations performed by: https://www.npmjs.com/package/memcached
The Node.js uses the mssql subtype for instrumenting database operations performed by: https://www.npmjs.com/package/mssql
The Node.js uses the graphql subtype for instrumenting graphql queries performed by https://www.npmjs.com/package/graphql (a common library used by different graphql implementations in Node.js)
If we're trying to have this reflect the current reality we'll want to get span_types.json updated.
|
Thanks @astorm and @trentm for the feedback: I've added For datastores that could be used both as a cache of a database like
I don't think it's worth adding duplicate entries for For |
I agree. (My guess is that this shouldn't be considered a compatibility issue if we change it, but I'm curious if others have opinions on that.) |
|
When we talk about "enforcing" this spec, what does that mean? Checking any type/subtype we use against this list to make sure it's "approved"? Just wanted to clarify before I open the Python issue for aligning. |
Yes, exactly this: enforcing the shared spec. Once checks starts to be enforced, the fun part of doing alignment can start :-). |
|
Merging spec as-is for now. ping @elastic/apm-agent-net to create an issue/PR to enforce the JSON specification . |
Adds a JSON spec + document status quo for known sub-types
Aims to document the status-quo, thus most span types & subtypes that are used by any agent should be reflected here.
Enforcing spec in tests should allow to opt-out for exact match to allow testing for:
Things to discuss
creating & moving dedicated spec for>> will be handled by follow-up issuestoragetype(s) ?type/subtypevalues that may be Java-only for now, but might be relevant later: should we include them right now ?app/inferredfor inferred spans captured through profilingprocessfor external command executioncustom/ (anything) for spans created through API/Annotations without explicittypesettype/subtypebe part of this shared spec ?db/h2database is Java-onlytemplateused for template engines is really platform-specific, but thetemplatetype should be commondb/hibernate-searchis Java-onlyBonus: should we have a dedicated>> will be handled by follow-up issueormtype ?orm/hibernate-searchinstead ofdb/hibernate-searchtemplatetype.Steps