Skip to content

Conversation

@Hweinstock
Copy link
Contributor

@Hweinstock Hweinstock commented Apr 29, 2025

Problem

A metric was forwarded from Flare with missing fields. This resulted in some confusion about why the metric wasn't showing up properly in Kibana.

Solution

  • refactor validation to check for missingFields and log a warning.
  • Ideally, we would throw in CI for these, but there is currently a significant number of metrics emitted without required fields.
  • add tests for this validation.

Future Work

  • Fix existing cases of emitting with missing required fields, so we can throw in CI when this happens.

  • Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
  • Your code changes must meet the guidelines in CONTRIBUTING.md.
  • License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions
Copy link

  • This pull request implements a feat or fix, so it must include a changelog entry (unless the fix is for an unreleased feature). Review the changelog guidelines.
    • Note: beta or "experiment" features that have active users should announce fixes in the changelog.
    • If this is not a feature or fix, use an appropriate type from the title guidelines. For example, telemetry-only changes should use the telemetry type.

@Hweinstock Hweinstock changed the title fix(telemetry): metrics with missing fields are not always emitted. (WIP) feat(telemetry): metrics with missing fields are warned in the logs. Apr 29, 2025
@Hweinstock Hweinstock marked this pull request as ready for review April 29, 2025 21:55
@Hweinstock Hweinstock requested a review from a team as a code owner April 29, 2025 21:55
@Hweinstock
Copy link
Contributor Author

tracked failing test #7187.

@jpinkney-aws jpinkney-aws deleted the branch aws:master May 1, 2025 03:02
@justinmk3 justinmk3 reopened this May 1, 2025
@justinmk3 justinmk3 changed the base branch from feature/hybridChat to master May 1, 2025 13:38

// TODO: there are many instances in the toolkit where we emit metrics with missing fields. If those can be removed, we can configure this to throw in CI.
if (metadata.missingFields) {
logger.warn(msgPrefix + `"${metricName} emitted with missing fields: ${metadata.missingFields}`, false)
Copy link
Contributor

Choose a reason for hiding this comment

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

any concerns of this spamming the users logs too much?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what I see after messing around in Q for a few minutes, so I am not too concerned.

2025-05-02 15:32:30.013 [warning] telemetry: invalid Metric: "languageServer_setup emitted with missing fields: id
2025-05-02 15:32:30.013 [warning] telemetry: invalid Metric: "languageServer_setup emitted with missing fields: id
2025-05-02 15:32:37.919 [warning] telemetry: invalid Metric: "languageServer_setup emitted with missing fields: id
2025-05-02 15:32:37.919 [warning] telemetry: invalid Metric: "languageServer_setup emitted with missing fields: id
2025-05-02 15:33:00.476 [warning] telemetry: invalid Metric: "amazonq_addMessage emitted with missing fields: cwsprChatTimeToFirstChunk
2025-05-02 15:34:09.534 [warning] telemetry: invalid Metric: "amazonq_addMessage emitted with missing fields: cwsprChatTimeToFirstChunk
2025-05-02 15:35:04.858 [warning] telemetry: invalid Metric: "amazonq_addMessage emitted with missing fields: cwsprChatTimeToFirstChunk
2025-05-02 15:36:13.968 [warning] telemetry: invalid Metric: "amazonq_addMessage emitted with missing fields: cwsprChatTimeToFirstChunk

I also checked Toolkit and wasn't able to find any of these. addMessage was fixed in aws/language-servers#1222. I can look into the languageServer_setup one as a follow-up.

Copy link
Contributor

Choose a reason for hiding this comment

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

we should probably do a logOnce thing for these warnings, since duplicates aren't useful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this pattern seems somewhat common, I added a utility to execute functions once per unique argument. Then used that to implement the logOnce. (also refactored the processUtils to use it).

* logOnce('test') // prints: test
* ```
*/
export function oncePerUniqueArg<T, U extends any[]>(
Copy link
Contributor

Choose a reason for hiding this comment

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

off-topic: Might be able to drop onceChanged in favor of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah its somewhat of a generalization. I think we could reimplement onceChanged to call oncePerUniqueArg with a buffer size of 1 so that it only compares against the previous.

@Hweinstock Hweinstock merged commit bb11bd9 into aws:master May 7, 2025
42 of 46 checks passed
@Hweinstock Hweinstock deleted the fix/validation branch May 7, 2025 16:14
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.

3 participants