-
Notifications
You must be signed in to change notification settings - Fork 21
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
Reduce and simplify error reporting service buckets #128
Conversation
utils/log-target.js
Outdated
@@ -27,6 +27,25 @@ const CDN_REGEX = new RegExp( | |||
'\\.ampproject\\.net/', | |||
'i' | |||
); | |||
const CHANNEL_TYPES = { |
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 notice these are mostly the same as
error-tracker/utils/human-rtv.js
Lines 16 to 32 in 7f607b3
const RELEASE_CHANNELS = { | |
'00': 'Experimental', | |
'01': 'Stable', | |
'02': 'Control', | |
'03': 'Beta', | |
'04': 'Nightly', | |
'05': 'Nightly-Control', | |
'10': 'Experiment-A', | |
'11': 'Experiment-B', | |
'12': 'Experiment-C', | |
'20': 'Inabox-Control-A', | |
'21': 'Inabox-Experiment-A', | |
'22': 'Inabox-Control-B', | |
'23': 'Inabox-Experiment-B', | |
'24': 'Inabox-Control-C', | |
'25': 'Inabox-Experiment-C', | |
}; |
Could we just reuse the same mapping? I would actually prefer to not mix Canary and RC, since we test really experimental features in Canary.
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 imagine your concern is being able to detect errors in Experimental features, and separate them out from the errors in Beta. With this grouping, that won't be a problem. My thinking around this is that the version names will already differentiate between Beta/Experimental. A single channel (ie. Experimental) can also be viewed using the built-in filtering mechanisms.
My reasoning for not wanting to use the same mapping is twofold
- It becomes redundant, as each service bucket will contain only the release matching its channel (with the exception of opt-in and percentage, which would be identical); this means we'd still have the giant list of services. Since older RTVs quickly drop off the list, that makes each service bucket contain only one or two versions, and we lose the benefit that service grouping provides.
- By grouping in this way, each service represents a specific commit in the code. Everything in Production is running from the same point, but with different configs; likewise for Development, Nightly, etc. The version dropdown then provides further refinement. By grouping Stable+Control, or Beta+Experimental, I think we'll get better signal from both, while maintaining the same ability to narrow down the list as desired.
WDYT?
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.
My thinking around this is that the version names will already differentiate between Beta/Experimental. A single channel (ie. Experimental) can also be viewed using the built-in filtering mechanisms.
Excellent, this was my concern.
It becomes redundant, as each service bucket will contain only the release matching its channel (with the exception of opt-in and percentage, which would be identical); this means we'd still have the giant list of services.
Is this level of detail even needed for the service name? Perhaps we should just group by CDN
and Prod
ness, and use the version level filtering?
If we do stick with this list, lets extract it and the one from human-rtv.js
into a common file.
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.
Done, and then collected the various RTV utils into one folder.
utils/log-target.js
Outdated
@@ -27,6 +27,25 @@ const CDN_REGEX = new RegExp( | |||
'\\.ampproject\\.net/', | |||
'i' | |||
); | |||
const CHANNEL_TYPES = { |
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.
My thinking around this is that the version names will already differentiate between Beta/Experimental. A single channel (ie. Experimental) can also be viewed using the built-in filtering mechanisms.
Excellent, this was my concern.
It becomes redundant, as each service bucket will contain only the release matching its channel (with the exception of opt-in and percentage, which would be identical); this means we'd still have the giant list of services.
Is this level of detail even needed for the service name? Perhaps we should just group by CDN
and Prod
ness, and use the version level filtering?
If we do stick with this list, lets extract it and the one from human-rtv.js
into a common file.
Co-Authored-By: Justin Ridgewell <justin@ridgewell.name>
Bucket names now take the form
(CDN|Origin) (Development|Production|Nightly|Experiments|Inabox-(Control|Experiment)-[ABC])
Expected errors get
(Expected)
appended to the bucket name. This allows the most common use cases to see errors grouped by the release type. More nuanced investigations, such as 1p/3p and single-pass/multi-pass/ESM can still be performed across individual service buckets, or across all of them, but using the regular filter field (ie. filtering by3p=1
oresm=1
orspt=mp
), without splitting up the main buckets any furtherPart of #112