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

Fix event tags for a couple of the methods provided by the Telemetry class #2963

Merged
merged 2 commits into from
May 23, 2024

Conversation

adrastogi
Copy link
Contributor

Summary of the pull request

The tags are missing from the Exception (and TimeTaken) logging helper methods in the Telemetry class. This change includes those entries in the data payload.

References and relevant issues

Detailed description of the pull request / Additional comments

I was looking through the class and noticed this, so I made the surgical fix for it. Theoretically, these events should be rewritten to follow the format we are trying to use for events now (e.g., as separate classes); that was a more involved change than I had time for due in part to the Exception class's use of additional internal methods... I figured that this is still a step in the right direction.

Validation steps performed

PR checklist

  • Closes #xxx
  • Tests added/passed
  • Documentation updated

Copy link
Contributor

@dhoehna dhoehna left a comment

Choose a reason for hiding this comment

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

The Priv A tags might change depending on when the telemetry was fired. I suggest to have Priv_A tags as an input parameter.

Further, ProductAndServiceUsage should not be used when we, the developers, can't trace the event to a specific user action.

An Event name should also be added as an input parameter.

@adrastogi
Copy link
Contributor Author

Spot on regarding the second point- these should be ProductAndServicePerformance, which I've fixed.

For the first point, my thought was that as these are related to durations and errors, the tags should be hard-coded... maybe I'm not imagining a scenario where a caller would want to override these.

For the third point, I guess the way this event is designed, all exception events are aggregated together, and rely on the action for bucketing- this seems ok to me, but let me know if you feel strongly about that.


In reply to: 2066638154

@dhoehna
Copy link
Contributor

dhoehna commented May 23, 2024

durations and errors,
I believe I understand your point of view. If an error occurs, most likely, we developers made a mistake.

I took a dive into the telemetry code and found LogError. LogError uses ProductAndServicePerformance.

If the DAS team hasn't said anything, I'm okay with the tag as ProductAndServicePerformance.

@adrastogi adrastogi merged commit c97ad88 into main May 23, 2024
4 checks passed
@adrastogi adrastogi deleted the user/adrastogi/fix-tags branch May 23, 2024 21:13
@krschau krschau added this to the Dev Home 0.15 milestone May 31, 2024
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.

None yet

3 participants