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

Add Tags.of method taking array of Pair #4960

Open
rafalh opened this issue Apr 11, 2024 · 6 comments
Open

Add Tags.of method taking array of Pair #4960

rafalh opened this issue Apr 11, 2024 · 6 comments
Labels
enhancement A general enhancement help wanted An issue that a contributor can help us with module: micrometer-core An issue that is related to our core module

Comments

@rafalh
Copy link

rafalh commented Apr 11, 2024

Please describe the feature request.

I would like to be able to provide metric tags as array of Pair objects

Rationale

Currently I have two ways to pass tags:

meterRegistry.counter(path, "foo", foo, "bar", bar).increment()                           // 1
meterRegistry.counter(path, Tags.of(Tag.of("foo", foo), Tag.of("bar", bar))).increment()  // 2

First method may lead to runtime errors if someone forgets to provide key or value resulting in uneven number of tag arguments.
Second method is too verbose.

I would like to be able to write in Kotlin:

meterRegistry.counter(path, Tags.of("foo" to foo, "bar" to bar)).increment()

Note: to operator in Kotlin creates a Pair object so this is equivalent to:

meterRegistry.counter(path, Tags.of(Pair("foo", foo), Pair("bar", bar))).increment()

Nice to have: additional methods in MeterRegistry class allowing to avoid dealing with Tags class - probably a breaking change?

meterRegistry.counter(path, "foo" to foo, "bar" to bar).increment()

Additionally it would be nice if Micrometer allowed passing values of any type and converting them to String automatically, e.g.:

meterRegistry.counter(path, Tags.of("foo" to 10)).increment()
@shakuzen
Copy link
Member

With reference to Pair, I guess this is a Kotlin-specific feature request. Could something like this be added with an extension function? I'm not very familiar with Kotlin, but if you have a concrete proposal that doesn't break compatibility, we could consider it.

Additionally it would be nice if Micrometer allowed passing values of any type and converting them to String automatically

I think we've had this request before, but I'm not sure I think it's something we should do. I don't think it should be Micrometer's responsibility to convert arbitrary values to strings. Micrometer should treat tag values as opaque so what users give Micrometer is what ends up in the metrics backend (other than limitations imposed by backends and handled by NamingConvention). If we are converting to strings, there's an opportunity for surprising and inconsistent results.

@shakuzen shakuzen added waiting for feedback We need additional information before we can continue and removed waiting-for-triage labels Apr 17, 2024
Copy link

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

@checketts
Copy link
Contributor

checketts commented Apr 25, 2024

@shakuzen This is absolutely achievable via an extension function:

fun MeterRegistry.counter(id: String, vararg tags: Pair<String, String>): Counter {
  return this.counter(id, tags.map { Tag.of(it.first, it.second) })
}

I also agree that Micrometer should only be accepting strings for tag and not convert an into to a string. If the user want a convenient conversion then they could call .toString(). There are too many ways the signature can become confusing if it is too broad. It also is a slippery way to have a cardinality explosion.

I would love for Kotlin extension function to be available to import, that way they are only visible to those users and not noise for the rest of the users.

Is there currently a Kotlin part of the build that would be a good place to put such extension functions?

@rafalh
Copy link
Author

rafalh commented Apr 30, 2024

@shakuzen This is absolutely achievable via an extension function:

fun MeterRegistry.counter(id: String, vararg tags: Pair<String, String>): Counter {
  return this.counter(id, tags.map { Tag.of(it.first, it.second) })
}

Yes, MeterRegistry functions can be implemented as extension functions. But sometimes it is useful to create Tags object and pass it to a function. Unfortunately static methods like Tags.of cannot be extended. I created tagsOf utility function in my project and that works pretty nice.

I would love for Kotlin extension function to be available to import, that way they are only visible to those users and not noise for the rest of the users.

It makes sense to me too to only expose them for Kotlin users. It's hard to imagine why non-Kotlin users would need to use Pair objects instead of Tag.

@jonatan-ivanov
Copy link
Member

Is there currently a Kotlin part of the build that would be a good place to put such extension functions?

I guess micrometer-core/src/main/kotlin (instrumentation for Kotlin).

My questions are:

  • Is it possible to do this without polluting the Java pieces?
    Having an "api" dependency on the Kotlin SDK is a no go for me
  • How should we handle Kotlin versions? (I guess this is a problem currently as well)

@checketts
Copy link
Contributor

That is perfect and using the same approach: add extension methods there that Kotlin users can import and Java users won't see.

As long as the extension methods are only leveraging core classes like Pair there shouldn't be any problem with Kotlin versions,

The Kotlin dependency should be an optional one that won't affect non-Kotlin users. compileOnly should work for this case.

@jonatan-ivanov jonatan-ivanov added help wanted An issue that a contributor can help us with enhancement A general enhancement module: micrometer-core An issue that is related to our core module and removed waiting for feedback We need additional information before we can continue labels May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A general enhancement help wanted An issue that a contributor can help us with module: micrometer-core An issue that is related to our core module
Projects
None yet
Development

No branches or pull requests

4 participants