Skip to content
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

bug: Fix metric message text for source of message #660

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jrconlin
Copy link
Member

Closes SYNC-4193

@jrconlin jrconlin requested review from pjenvey and taddes March 14, 2024 19:21
Copy link
Member

@pjenvey pjenvey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way I'm understanding this metric it seems to have the correct name: it's identical to the other message_data metric emitted by incr_success_metrics. They're both called when a message has been successfully delivered, either via webpush or an external bridge.

If anything maybe we should be calling incr_success_metrics here but we reserve its notification.bridge.sent metric solely for external bridges (not webpush). Its third metric notification.total_request_time we emit elsewhere in this module (for some reason, only after trigger_notification_check. I think we forgot to add the same metric for after a successful send_notification call).

I think this should probably be calling a new common function that solely emits the message_data metric that incr_success_metrics would also call? Then both versions would include the same tags (they currently don't).

@jrconlin
Copy link
Member Author

Huh. Good call on having a common function that emits the set of metrics. I'll dig into that.

Mostly, this was just me trying to fix up some of the weird discrepancies around metric calls so centralizing them a bit will help.

@jrconlin jrconlin marked this pull request as draft May 14, 2024 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants