-
Notifications
You must be signed in to change notification settings - Fork 150
Allow exporting JS metadata #318
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
base: main
Are you sure you want to change the base?
Allow exporting JS metadata #318
Conversation
// NewJszCollector creates a new NATS JetStream Collector. | ||
func NewJszCollector( | ||
endpoint, prefix string, | ||
servers []*CollectedServer, | ||
streamMetaKeys, consumerMetaKeys []string, | ||
) prometheus.Collector { | ||
return newJszCollector(getSystem(JetStreamSystem, prefix), endpoint, servers, streamMetaKeys, consumerMetaKeys) | ||
} |
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.
There is a separate code path now for JetStream collectors to be able to supply the meta keys. This seemed better than to pass the arguments around for the other collectors where they wouldn't be used.
This makes JS collectors somewhat of a special case in the code, not sure if you like it that way.
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.
I don't love it but it's probably the best option given the current organization of the code.
for _, k := range streamMetaKeys { | ||
streamLabels = append(streamLabels, "stream_meta_"+k) | ||
} |
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.
Stream metadata is exported as labels named stream_meta_…
, making sure they don't conflict with consumer metadata defined below.
Maybe it would actually be nice to combine these, WDYT?
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.
I think it is probably logical to keep them separated. There is potential for high cardinality but it would be self-imposed based on your own provided keys so that seems ok to me.
a853ce9
to
8ebe945
Compare
bedba8f
to
fbde41a
Compare
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.
Thanks for the contribution; apologies for the long tail on this. Just a few comments
for _, k := range streamMetaKeys { | ||
streamLabels = append(streamLabels, "stream_meta_"+k) | ||
} |
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.
I think it is probably logical to keep them separated. There is potential for high cardinality but it would be self-imposed based on your own provided keys so that seems ok to me.
// NewJszCollector creates a new NATS JetStream Collector. | ||
func NewJszCollector( | ||
endpoint, prefix string, | ||
servers []*CollectedServer, | ||
streamMetaKeys, consumerMetaKeys []string, | ||
) prometheus.Collector { | ||
return newJszCollector(getSystem(JetStreamSystem, prefix), endpoint, servers, streamMetaKeys, consumerMetaKeys) | ||
} |
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.
I don't love it but it's probably the best option given the current organization of the code.
suggestion in PR 318
suggestion in PR 318
suggestion in PR 318
do not reify extraction functions, just pass keys around suggestion in PR 318
add new cmd line opts
Allow exporting selected JS metadata, e.g. to route alerts based on that.
Implementation of #317