-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[BUGFIX] Fix usage stats events #3857
[BUGFIX] Fix usage stats events #3857
Conversation
✔️ Deploy Preview for niobium-lead-7998 ready! 🔨 Explore the source changes: 5f579f5 🔍 Inspect the deploy log: https://app.netlify.com/sites/niobium-lead-7998/deploys/61ba2639b576860007042bbc 😎 Browse the preview: https://deploy-preview-3857--niobium-lead-7998.netlify.app |
HOWDY! This is your friendly 🤖 CHANGELOG bot 🤖Please don't forget to add a clear and succinct description of your change under the Develop header in ✨ Thank you! ✨ |
@@ -187,7 +189,7 @@ def build_envelope(self, message): | |||
+ "Z" | |||
) | |||
|
|||
event_duration_property_name: str = f'{message["event_name"]}.duration'.replace( | |||
event_duration_property_name: str = f'{message["event"]}.duration'.replace( |
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.
@anthonyburdi I am sure you are right -- it just seems a little surprising that it has worked with our "standard" tests before (with the key being "event_name") -- are you sure about this change?
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 thought so too, but this was a recent addition in #3729 - we have used event_name
as a parameter in many places but the key has always been event
.
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.
@anthonyburdi So maybe that was the issue, meaning that while the schema and message tests worked, the events were never actually emitted?.. Is that what is happening here?
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 one question there...
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.
LGTM
Changes proposed in this pull request:
UsageStatisticsHandler.emit
, preventing events from being sentDefinition of Done
Please delete options that are not relevant.
Thank you for submitting!