-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: propagate Publish
caller through all pubsub metrics
#2231
Conversation
-- migrate:up | ||
|
||
ALTER TABLE topic_events | ||
ADD COLUMN caller TEXT NOT NULL; |
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.
This will need to have a default (or be nullable), otherwise the migration will fail on existing columns.
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.
Whoops thanks for the reminder!
pubsubTopicNameAttr = "ftl.pubsub.topic.name" | ||
pubsubCallerVerbNameAttr = "ftl.pubsub.publish.caller.verb.name" | ||
pubsubTopicRefAttr = "ftl.pubsub.topic.ref" | ||
pubsubTopicModuleAttr = "ftl.pubsub.topic.module.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.
How is this different to .ref above?
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.
They're the same, but the consensus in Slack (with the rest of the otel folks) was to just log both. It is possible to filter by module-only using the ref by doing a string match in datadog, but that's a little more annoying than having this attr here. WDYT? I could just remove the duplicated module attrs everywhere.
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.
Oh hmm it looks like DD's "split graph" feature doesn't support any special string matching; you have to just pick an attribute and it slices all the graphs by that. So that would be an argument to keep module in there :/
f0d5f07
to
06fced5
Compare
Previously, only
ftl.pubsub.published
had the caller attribute. This PR adds caller to the rest of the pubsub metrics and also slightly restructures the topic name/ref logging to be more consistent with the rest of this fileIssue: #2194