DM-46821: Refactor metrics configuration #311
Merged
Conversation
This file contains 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
rra
force-pushed
the
tickets/DM-46821
branch
5 times, most recently
from
242c68f
to
a0fe58d
Compare
Rework the metrics configuration so that `MetricsConfiguration` is a union type supporting either a disabled or a Kafka-based metrics configuration, which can be used as a drop-in type in an application `BaseSettings` class and will automatically select one or the other based on what configuration is available. Have those two configuration classes in turn create two different `EventManager` classes that provide the same interface, and two different underlying `EventPublisher` classes. Use this to implement the no-op metrics functionality rather than checking a boolean. This allows all of the Kafka configuration to be omitted if metrics are disabled. Update the documentation accordingly, and fix a few minor problems with the documentation discovered as part of the Gafaelfawr integration. Move the `app_name` field to the top level of the metrics configuration on the grounds that this will likely apply to any application metrics, including things that are not events. Rename `metrics_events` to just `events`, since the metrics part is clear in context. Use the `topic_prefix` name uniformly instead of `base_topic_prefix`.
`application` looks a bit nicer in YAML configuration since it doesn't require handling camel-case transformations.
We will get rid of this code as soon as there is a better solution in dataclasses-avroschema.
fajpunk
approved these changes
Oct 18, 2024
Be clearer about what the name passed to `build_validated_model` in the `EventManager` class is used for.
fajpunk
approved these changes
Oct 18, 2024
Refactor the `EventManager` classes to share more structural code with subclasses. The duplicate detection and iniitalization detection code has moved to the parent class, and subclasses now only provide the code to create a publisher from an enriched model, and can override initialize and aclose if needed.
fajpunk
reviewed
Oct 18, 2024
The logic for event manager loggers is common to all of the implementations, so move it into the parent class.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Rework the metrics configuration so that
MetricsConfigurationis a union type supporting either a disabled or a Kafka-based metrics configuration, which can be used as a drop-in type in an applicationBaseSettingsclass and will automatically select one or the other based on what configuration is available. Have those two configuration classes in turn create two differentEventManagerclasses that provide the same interface, and two different underlyingEventPublisherclasses. Use this to implement the no-op metrics functionality rather than checking a boolean. This allows all of the Kafka configuration to be omitted if metrics are disabled.Update the documentation accordingly, and fix a few minor problems with the documentation discovered as part of the Gafaelfawr integration.
Move the
app_namefield to the top level of the metrics configuration on the grounds that this will likely apply to any application metrics, including things that are not events. Renamemetrics_eventsto justevents, since the metrics part is clear in context.Use the
topic_prefixname uniformly instead ofbase_topic_prefix.