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

Allow to export configurable set of topic configuration keys #97

Merged
merged 4 commits into from
Jun 22, 2021

Conversation

amuraru
Copy link
Contributor

@amuraru amuraru commented Jun 21, 2021

Added a new configuration parameter to allow defining the
set of topic configuration keys that should be exported as label pairs
in kafka_topic_info

By default only cleanup.policy configuration is exported

See https://kafka.apache.org/documentation/#topicconfigs
for the list of topic level configs that can be defined

@@ -20,7 +20,6 @@ func (s *Service) GetTopicConfigs(ctx context.Context) (*kmsg.DescribeConfigsRes
resourceReq := kmsg.NewDescribeConfigsRequestResource()
resourceReq.ResourceType = kmsg.ConfigResourceTypeTopic
resourceReq.ResourceName = topic.Topic
resourceReq.ConfigNames = []string{"cleanup.policy"}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is done to ensure all configurations are returned

@weeco
Copy link
Contributor

weeco commented Jun 22, 2021

Hey Adi,
we are kinda abusing the prometheus labels for that. Someone else might be interested in other topic config options and we can definetely not add all config options. Hence I thought about that we could maybe make them configurable? I'm unsure whether (and how) it is possible to make dynamic labels work with the Prometheus lib.

Basically making it configurable like this (and use a sane default there):

  topics:
    # Granularity can be per topic or per partition. If you want to reduce the number of exported metric series and
    # you aren't interested in per partition metrics you could choose "topic".
    granularity: partition
    # AllowedTopics are regex strings of topic names whose topic metrics that shall be exported.
    # You can specify allowed topics by providing literals like "my-topic-name" or by providing regex expressions
    # like "/internal-.*/".
    allowedTopics: [ ".*" ]
    # IgnoredTopics are regex strings of topic names that shall be ignored/skipped when exporting metrics. Ignored topics
    # take precedence over allowed topics.
    ignoredTopics: [ ]
    # infoMetric is a configuration object for the kminion_kafka_topic_info metric
    infoMetric:
      # ConfigKeys are regex strings of Topic configs that you want to have exported as part of the metric
      configKeys: ["max_message_bytes", "min_insync_replicas"]

What do you think?

@amuraru
Copy link
Contributor Author

amuraru commented Jun 22, 2021

we are kinda abusing the prometheus labels for that. Someone else might be interested in other topic config options and we can definetely not add all config options. Hence I thought about that we could maybe make them configurable? I'm unsure whether (and how) it is possible to make dynamic labels work with the Prometheus lib.

makes a lot of sense, Martin!

Will amend this PR to have these configurable.
Any preference for the defaults in configKeys ? Should I keep cleanup.policy only as this is what the tool supports?

@weeco
Copy link
Contributor

weeco commented Jun 22, 2021

I think cleanup.policy only should be fine for most users. If someone wants to inspect topic configs I'd hope they would use a UI like Kowl for it where users can also see what's inherited / default etc like that:

image

Thanks for tackling this :)

@amuraru amuraru force-pushed the moretopicconfigs branch from 9e36bda to 89ece71 Compare June 22, 2021 16:39
@amuraru
Copy link
Contributor Author

amuraru commented Jun 22, 2021

Kowl is awesome indeed!
One particular use-case for exposing these as metrics is when we want for example to track the evolution of a particular topic configurations. Say, when was the topic max.message.size changed.

@@ -55,7 +56,7 @@ func (e *Exporter) collectTopicInfo(ctx context.Context, ch chan<- prometheus.Me
e.logger.Warn("failed to get metadata of a specific topic",
zap.String("topic_name", topic.Topic),
zap.Error(typedErr))
return false
continue
Copy link
Contributor Author

Choose a reason for hiding this comment

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

be lenient to individual topic failures

@amuraru
Copy link
Contributor Author

amuraru commented Jun 22, 2021

Updated the PR based on your suggestion @weeco
Let me know when you get a chance to review

@amuraru amuraru changed the title Export more topic configuration parameters Allow to export configurable set of topic configuration keys Jun 22, 2021
docs/reference-config.yaml Outdated Show resolved Hide resolved
charts/kminion/values.yaml Outdated Show resolved Hide resolved
Added a new configuration parameter to allow defining the
set of topic configuration keys that should be exported as label pairs
in kafka_topic_info

By default only `cleanup.policy` configuration is exported

See https://kafka.apache.org/documentation/#topicconfigs
for the list of topic level configs that can be defined
@amuraru amuraru force-pushed the moretopicconfigs branch from 7eb49d6 to 77249c3 Compare June 22, 2021 16:48
Copy link
Contributor

@weeco weeco left a comment

Choose a reason for hiding this comment

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

Generally LGTM, added a few comments though 👍 .

minion/config_topic_config.go Outdated Show resolved Hide resolved
minion/config_topic_config.go Outdated Show resolved Hide resolved
minion/config_topic_config.go Outdated Show resolved Hide resolved
amuraru and others added 3 commits June 22, 2021 23:15
Co-authored-by: Martin Schneppenheim <weeco91@gmail.com>
Co-authored-by: Martin Schneppenheim <weeco91@gmail.com>
Co-authored-by: Martin Schneppenheim <weeco91@gmail.com>
@weeco weeco merged commit 63cba76 into redpanda-data:master Jun 22, 2021
@amuraru amuraru deleted the moretopicconfigs branch July 1, 2021 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants