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

feat(googlecloudmonitoring): support monitoring filters #37264

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

chenlujjj
Copy link
Contributor

Description

Link to tracking issue

Fixes part of #36898

Testing

Documentation

@chenlujjj chenlujjj force-pushed the feat-googlecloudmonitoring-filter branch from 1cc5e3f to 81dff4e Compare January 16, 2025 09:04
@chenlujjj chenlujjj changed the title feat(googlecloudmonitoring): filter metric via regex feat(googlecloudmonitoring): support monitoring filters Jan 17, 2025
@chenlujjj chenlujjj force-pushed the feat-googlecloudmonitoring-filter branch from acf2409 to 31522d0 Compare January 17, 2025 07:44
@chenlujjj chenlujjj force-pushed the feat-googlecloudmonitoring-filter branch from 31522d0 to 6fd50f2 Compare January 17, 2025 07:46

One of `metric_name` and `monitoring_filter` MUST be specified, but should not be specified at the same time.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
One of `metric_name` and `monitoring_filter` MUST be specified, but should not be specified at the same time.
One of `metric_name` and `monitoring_filter` MUST be specified, but MUST not be specified at the same time.


One of `metric_name` and `monitoring_filter` MUST be specified, but should not be specified at the same time.
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on https://cloud.google.com/monitoring/api/v3/filters, if we are applying these to only to ListMetricDescriptor, there are a lot of restrictions on what you can filter by that might confuse users. They can't use resource or group selectors, and presumably can't filter on labels: metric.labels.instance_name = monitoring.regex.full_match("gke-(hipster|nginx).*").

One solution would be to make this a metric_descriptor_filter config option, so that it is clear we are filtering metric descriptors (and we should also document that there are restrictions on that filter). That way we could later add a separate time_series_filter that we add to actual ListTimeSeries queries if we want.

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.

3 participants