-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
chore(observability): remove more deprecated internal metrics #17542
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,15 @@ pub(crate) struct ThrottleEventDiscarded { | |
|
||
impl InternalEvent for ThrottleEventDiscarded { | ||
fn emit(self) { | ||
debug!(message = "Rate limit exceeded.", key = ?self.key); // Deprecated. | ||
// TODO: Technically, the Component Specification states that the discarded events metric | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, yeah, this is a good question. I wonder how much harm there is in allowing additional tags as long as the required tags are there. |
||
// must _only_ have the `intentional` tag, in addition to the core tags like | ||
// `component_kind`, etc, and nothing else. | ||
// | ||
// That doesn't give us the leeway to specify which throttle bucket the events are being | ||
// discarded for... but including the key/bucket as a tag does seem useful and so I wonder | ||
// if we should change the specification wording? Sort of a similar situation to the | ||
// `error_code` tag for the component errors metric, where it's meant to be optional and | ||
// only specified when relevant. | ||
Comment on lines
+12
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, the spec should be loosened there. |
||
counter!( | ||
"events_discarded_total", 1, | ||
"key" => self.key, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
--- | ||
date: "2023-07-05" | ||
title: "0.31 Upgrade Guide" | ||
description: "An upgrade guide that addresses breaking changes in 0.31.0" | ||
authors: ["tobz"] | ||
release: "0.31.0" | ||
hide_on_release_notes: false | ||
badges: | ||
type: breaking change | ||
--- | ||
|
||
Vector's 0.31.0 release includes **breaking changes**: | ||
|
||
1. [Removal of various deprecated internal metrics](#deprecated-internal-metrics) | ||
|
||
We cover them below to help you upgrade quickly: | ||
|
||
## Upgrade guide | ||
|
||
### Breaking changes | ||
|
||
#### Removal of various deprecated internal metrics {#deprecated-internal-metrics} | ||
|
||
Over the course of many of the previous releases, we've been working to deprecate the usage of older | ||
internal metrics as we worked towards implementing full support for the [Component | ||
Specification][component_spec], which dictates the basic metrics that all components, or the basic | ||
metrics all components of a specific type, are expected to emit. | ||
|
||
We've made enough progress on this work that we've gone ahead and removed many of the deprecated | ||
metrics from this release. First, below is a list of all metrics we've removed: | ||
|
||
- `events_in_total` (superceded by `component_received_events_total`) | ||
- `events_out_total` (superceded by `component_sent_events_total`) | ||
- `processed_bytes_total` (superceded by either `component_received_bytes_total` or | ||
`component_sent_bytes_total`, more below) | ||
- `processed_events_total` (superceded by either `component_received_events_total` or | ||
`component_sent_events_total`, more below) | ||
- `processing_errors_total` (superceded by `component_errors_total`) | ||
- `events_failed_total` (superceded by `component_errors_total`) | ||
|
||
Most of the removals have straightforward replacements, but the `processed_`-prefixed metrics | ||
involve a small amount of logic. For **sources**, `processed_bytes_total` is superceded by | ||
`component_received_bytes_total`, and `processed_events_total` is superceded by | ||
`component_received_events_total`. For **sinks**, `processed_bytes_total` is superceded by | ||
`component_sent_bytes_total`, and `processed_events_total` is superceded by | ||
`component_sent_events_total`. | ||
|
||
A small note is that a small number of components still emit some of these metrics, as they provided | ||
additional tags and information that is disallowed by the Component Specification. They will be | ||
removed in a future version once we can rectify those discrepancies, but they are effectively | ||
removed as of this release: you cannot depend on them still existing. | ||
Comment on lines
+48
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note this may change if we elect to update the spec. |
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.
Not relevant to this PR, but it does strike me that maybe
component_errors_total
would also be a good candidate for shared internal event type that could assert all of the required fields are present.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.
Yeah, fair point.
There's still a lot of custom error metrics that are marked as deprecated in favor of
component_errors_total
... the work to clean those up would probably be a good spot to explore moving this stuff into a single, shared event type.