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

🔉 monitor reported errors #2335

Merged
merged 2 commits into from
Jul 11, 2023
Merged

🔉 monitor reported errors #2335

merged 2 commits into from
Jul 11, 2023

Conversation

bcaudan
Copy link
Contributor

@bcaudan bcaudan commented Jul 11, 2023

Motivation

Have an idea on how frequently we are reporting SDK errors to customers.
For now, we only report when:

  • an event limiter threshold is reached
  • retry strategy queue was full

Changes

Send telemetry debug when reporting an error

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@bcaudan bcaudan requested review from a team as code owners July 11, 2023 12:20
@bcaudan
Copy link
Contributor Author

bcaudan commented Jul 11, 2023

/to-staging

@dd-devflow
Copy link
Contributor

dd-devflow bot commented Jul 11, 2023

🚂 Branch Integration: starting soon, merge in < 8m

commit 5f49439bfe will soon be integrated into staging-28.

This build is going to start soon! (estimated merge in less than 8m)

you can cancel this operation by commenting your pull request with /to-staging -c!

@dd-devflow
Copy link
Contributor

dd-devflow bot commented Jul 11, 2023

🚂 Branch Integration: this commit was successfully integrated

commit 5f49439bfe has been merged into staging-28 in merge commit d4561f4392

@bcaudan
Copy link
Contributor Author

bcaudan commented Jul 11, 2023

/to-staging

@dd-devflow
Copy link
Contributor

dd-devflow bot commented Jul 11, 2023

🚂 Branch Integration: starting soon, merge in < 8m

commit fc9509a5f3 will soon be integrated into staging-28.

This build is going to start soon! (estimated merge in less than 8m)

you can cancel this operation by commenting your pull request with /to-staging -c!

@codecov-commenter
Copy link

Codecov Report

Merging #2335 (fc9509a) into main (33c6330) will decrease coverage by 0.05%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##             main    #2335      +/-   ##
==========================================
- Coverage   94.20%   94.16%   -0.05%     
==========================================
  Files         206      206              
  Lines        6128     6131       +3     
  Branches     1357     1357              
==========================================
  Hits         5773     5773              
- Misses        355      358       +3     
Impacted Files Coverage Δ
packages/rum-core/src/boot/startRum.ts 89.79% <0.00%> (-1.88%) ⬇️
packages/rum/src/boot/startRecording.ts 90.90% <0.00%> (-4.33%) ⬇️
packages/logs/src/boot/startLogs.ts 83.78% <50.00%> (-2.33%) ⬇️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mimfgg
Copy link
Contributor

mimfgg commented Jul 11, 2023

/to-staging -c

@dd-devflow
Copy link
Contributor

dd-devflow bot commented Jul 11, 2023

🚂 Branch Integration: This merge request build was cancelled

This merge request build was cancelled

@mimfgg
Copy link
Contributor

mimfgg commented Jul 11, 2023

/to-staging

@dd-devflow
Copy link
Contributor

dd-devflow bot commented Jul 11, 2023

🚂 Branch Integration: starting soon, merge in < 8m

commit fc9509a5f3 will soon be integrated into staging-28.

This build is going to start soon! (estimated merge in less than 8m)

you can cancel this operation by commenting your pull request with /to-staging -c!

@mimfgg
Copy link
Contributor

mimfgg commented Jul 11, 2023

(re queueing this pull request, this workflow was cancelled due to a temporal update)

@dd-devflow
Copy link
Contributor

dd-devflow bot commented Jul 11, 2023

🚂 Branch Integration: this commit was successfully integrated

commit fc9509a5f3 has been merged into staging-28 in merge commit 053a7d2aaa

@@ -51,6 +52,8 @@ export function startLogs(
status: StatusType.error,
},
})
addTelemetryDebug('Error reported to customer', { 'error.message': error.message })
Copy link
Member

Choose a reason for hiding this comment

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

💬 suggestion: ‏the three added telemetry logs are identical. Maybe add the error source in the context? It could be logs / rum / replay

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 looking at the possible values of error.message and service, we should be able to identify exactly from where each log is coming.
Do you have a use case in mind that would benefit from adding this extra attribute?

Copy link
Member

Choose a reason for hiding this comment

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

I don't! service is fine to distinghuish between RUM and Logs, but not replay. If you think we don't need to run some analytics between RUM and Replay, we can certainly rely on the message to figure out what was the source of the log.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you think we don't need to run some analytics between RUM and Replay, we can certainly rely on the message

That sounds fine for now, let's wait and see if we feel the need to add more context later

@bcaudan bcaudan merged commit 92830a4 into main Jul 11, 2023
@bcaudan bcaudan deleted the bcaudan/monitor-limit-reached branch July 11, 2023 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants