-
Notifications
You must be signed in to change notification settings - Fork 195
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
Clarify notes on system-specific databases for operation and collection names #1863
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 5 out of 8 changed files in this pull request and generated no comments.
Files not reviewed (3)
- docs/database/cassandra.md: Evaluated as low risk
- docs/database/cosmosdb.md: Evaluated as low risk
- docs/database/mongodb.md: Evaluated as low risk
555a5ff
to
2981273
Compare
note: | | ||
It is RECOMMENDED to capture the value as provided by the application without attempting to do any case normalization. | ||
|
||
For batch operations, if the individual operations are known to have the same collection name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the other example, are say it should be prepended by BATCH
, MULTI
, PIPELINE
, etc. Isn't there anything here to indicate the same? Can't we use BATCH
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried to adjust generic BATCH
recommendation to redis - it does not use term batching, but it has transactions (MULTI
/EXEC
command) and pipelining when multiple commands are sent one after another without awaiting result and them results are returned once pipeline is executed.
e.g.
pipe = jedis.pipelined();
Response<String> resp0 = pipe.get("seat:0"); // this is a promise, not the actual response
Response<String> resp3 = pipe.get("seat:3"); // commands could be the same or different
Response<String> resp4 = pipe.get("seat:4");
pipe.sync(); // this is where things are going to happen and what would be called `PIPELINE` operation.
and for transactions
jedis.multi();
trans.incrBy("counter:1", 1); // commands could be the same or different
trans.incrBy("counter:2", 2);
trans.incrBy("counter:3", 3);
trans.exec(); // this would be `MULTI` operation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any experts on Redis around here to let us know if creating a distinction between these cases (pipeline, transactions) is helpful? 😅
in general I think having some indication that was a "group" (batch/multi/etc) operation would be helpful
@@ -2,7 +2,6 @@ groups: | |||
- id: trace.db.common.minimal | |||
extends: attributes.db.client.minimal | |||
type: attribute_group | |||
stability: experimental |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to confirm, all these traces attributes are not experimental
? are they stable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great question. attributes groups are weird. They are not a telemetry signal - they just group attributes.
Attributes, metrics, spans, resources all require stability property, but it means nothing on the attribute groups.
It's not documented in the https://github.com/open-telemetry/semantic-conventions/blob/main/docs/general/semantic-convention-groups.md#semantic-convention-groups:
Groups that have attribute_group type do not describe semantic convention and are used for auxiliary purposes.
But if you think it's confusing or a bad idea to skip stability for them in some other way, please let me know, I want to hear it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My confusion was mostly because the traces got their tag removed and the span was updated from experimental
to development
, but I might have missed some info about how this distinction was made.
That was pretty much me trying to find out how to check what should be stable
vs experimental
vs development
Fixes #1573
Update
db.collection.name
anddb.operation.name
notes when database does not support cross-table query or multiple operations in the same query.Merge requirement checklist
[chore]