RUM-1236: Use enum for HTTP method parameter of RumMonitor#startResource API#1653
Merged
Conversation
xgouchet
suggested changes
Oct 3, 2023
0xnm
force-pushed
the
nogorodnikov/rum-1236/use-enum-for-http-method-arg-in-rummonitor-startresource-api
branch
from
October 3, 2023 11:39
2232fa3 to
da88bc3
Compare
xgouchet
previously approved these changes
Oct 3, 2023
0xnm
force-pushed
the
nogorodnikov/rum-1236/use-enum-for-http-method-arg-in-rummonitor-startresource-api
branch
from
October 3, 2023 12:54
da88bc3 to
8374551
Compare
0xnm
force-pushed
the
nogorodnikov/rum-1236/use-enum-for-http-method-arg-in-rummonitor-startresource-api
branch
from
October 3, 2023 12:55
8374551 to
5bcb223
Compare
0xnm
marked this pull request as ready for review
October 3, 2023 12:55
xgouchet
approved these changes
Oct 3, 2023
xgouchet
left a comment
xgouchet
left a comment
Contributor
There was a problem hiding this comment.
Good work indeed, this will limit us spamming the telemetry and can help guide the customers in using the proper values
Codecov Report
@@ Coverage Diff @@
## release/2.2.0 #1653 +/- ##
=================================================
+ Coverage 83.58% 83.68% +0.09%
=================================================
Files 457 458 +1
Lines 15734 15762 +28
Branches 2349 2352 +3
=================================================
+ Hits 13151 13189 +38
+ Misses 1954 1948 -6
+ Partials 629 625 -4
|
mariusc83
approved these changes
Oct 3, 2023
0xnm
deleted the
nogorodnikov/rum-1236/use-enum-for-http-method-arg-in-rummonitor-startresource-api
branch
October 3, 2023 14:40
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
In the RumMonitor#startResource API we accept HTTP methods as String, however this leads to many errors reported in telemetry from customers who try to supply freeform values for this parameter, because they cannot be matched to the RUM Schema enum. It means we also need to use enum, which aligns closer with what iOS has - similar API doesn’t allow freeform HTTP methods and in case of conversion to RUM schema enum fallback to get is used.
It may be some discrepancy if span and RUM resource reported for the same call, because in case of span freeform
http.methodis allowed and RUM resource expects the value from the enum (so if value cannot be matched,GETwill be used), but anyway it is the same what iOS SDK has right now, and at least RUM resource will be reported instead of loosing it due to exception.Review checklist (to be filled by reviewers)