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(encoding)!: EncodeLabelSet::encode() uses reference #257

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

Conversation

cratelyn
Copy link
Contributor

this commit alters the signature of the EncodeLabelSet::encode() trait method, such that it now accepts a mutable reference to its encoder.

this is related to #135, and is a second proposal following previous work in #240.

this change permits distinct label sets to be composed together, now that the label set encoder is not consumed. a new implementation for tuples (A, B) is provided.

this commit includes a test case showing that a metric family can compose two label sets together, and that such a family can successfully be digested by the python client library.

derive-encode is altered to generate code matching this new trait signature, and has been bumped to version 0.5.0 as a result of this breaking change in the prometheus-client library.

@cratelyn cratelyn force-pushed the encode-mut-ref.deux-label-set-only branch from 1ca988b to e281ace Compare January 21, 2025 03:07
@cratelyn cratelyn marked this pull request as draft January 21, 2025 03:08
@cratelyn cratelyn force-pushed the encode-mut-ref.deux-label-set-only branch from e281ace to 5d6c169 Compare January 21, 2025 03:15
@cratelyn cratelyn marked this pull request as ready for review January 21, 2025 03:16
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Very well done @cratelyn! Thank you.

@olix0r want to give this a review as well? Does this address your issue mentioned in #240 (comment)? Are there any other (breaking) changes you would need for linkerd?

src/encoding/text.rs Outdated Show resolved Hide resolved
Copy link

@olix0r olix0r left a comment

Choose a reason for hiding this comment

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

lgtm! thanks @cratelyn @mxinden 🌮🌮

this commit alters the signature of the `EncodeLabelSet::encode()` trait
method, such that it now accepts a mutable reference to its encoder.

this is related to prometheus#135, and is a second proposal following
previous work in prometheus#240.

this change permits distinct label sets to be composed together, now
that the label set encoder is not consumed. a new implementation for
tuples `(A, B)` is provided.

this commit includes a test case showing that a metric family can
compose two label sets together, and that such a family can successfully
be digested by the python client library.

`derive-encode` is altered to generate code matching this new trait
signature, and has been bumped to version 0.5.0 as a result of this
breaking change in the `prometheus-client` library.

Signed-off-by: katelyn martin <[email protected]>
@cratelyn cratelyn force-pushed the encode-mut-ref.deux-label-set-only branch from 5d6c169 to 98bca72 Compare January 22, 2025 16:13
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.

3 participants