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

Fixes #37647 - Make telemetry allowed_labels configurable #10243

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

pmoravec
Copy link
Contributor

It might not be the best idea to put the lengthy list of classes to settings.yaml. As the list contains majority of classes either way, maybe we can have there simply:

:class:
 - '.*'

?

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I think we shouldn't require those values to always be present in settings.yaml but rather have a hard coded default. We do that in config/settings.rb.

config/initializers/5_telemetry.rb Outdated Show resolved Hide resolved
@pmoravec
Copy link
Contributor Author

I think we shouldn't require those values to always be present in settings.yaml but rather have a hard coded default. We do that in config/settings.rb.

Good point. And we should allow to configure either :controller or :action or :class independently. Like allow in config:

:allowed_labels:
  :class:
   - '.*'

that will update just the classes.

I will update PR accordingly.

@pmoravec pmoravec force-pushed the telemetry-allowed-labels-to-settings branch from ae09483 to de95d06 Compare July 16, 2024 12:04
@pmoravec pmoravec changed the title Fixes #37647 - Move telemetry allowed_labels to settings Fixes #37647 - Make telemetry allowed_labels configurable Jul 16, 2024
@@ -125,4 +125,5 @@
'Widget',
],
}
allowed_labels.merge!(SETTINGS[:telemetry][:allowed_labels]) if SETTINGS[:telemetry] and SETTINGS[:telemetry][:allowed_labels]
Copy link
Member

Choose a reason for hiding this comment

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

Note this is a merge, not a deep merge. So it replaces all keys. Did you intend:

Suggested change
allowed_labels.merge!(SETTINGS[:telemetry][:allowed_labels]) if SETTINGS[:telemetry] and SETTINGS[:telemetry][:allowed_labels]
allowed_labels.deep_merge!(SETTINGS[:telemetry][:allowed_labels]) if SETTINGS.dig(:telemetry, :allowed_labels)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good points, I am still ruby newbie :)

@pmoravec pmoravec force-pushed the telemetry-allowed-labels-to-settings branch from de95d06 to 3d2b5cf Compare July 16, 2024 14:15
Signed-off-by: Pavel Moravec <pmoravec@redhat.com>
@pmoravec pmoravec force-pushed the telemetry-allowed-labels-to-settings branch from 3d2b5cf to 4cbcd5b Compare July 16, 2024 14:28
@pmoravec
Copy link
Contributor Author

I think the failing test:functionals are independent on my PR.

@ekohl
Copy link
Member

ekohl commented Jul 18, 2024

Correct, I just restarted those tests because it should have been fixed.

@pmoravec pmoravec requested a review from ekohl July 24, 2024 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants