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

KafkaMetrics - renameTag with "client.id" no longer works instead "client-id" #2256

Closed
3ygun opened this issue Sep 8, 2020 · 4 comments · Fixed by #2316
Closed

KafkaMetrics - renameTag with "client.id" no longer works instead "client-id" #2256

3ygun opened this issue Sep 8, 2020 · 4 comments · Fixed by #2316
Labels
release notes Noteworthy change to call out in the release notes
Milestone

Comments

@3ygun
Copy link

3ygun commented Sep 8, 2020

Hey, I'm upgrading our application from Spring Boot 2.1.X to 2.3.3 and bumped our Micrometer version from 1.3.2 to 1.5.4 (Also bumping Spring Cloud Streams from Greenwich.RC2 to Hoston.SR8). In doing these upgrades I noticed that we were logging a ton of (lines for each kafka meter):

2020-09-08 12:31:59.667 INFO  [pool-16-thread-1] io.micrometer.core.instrument.binder.kafka.KafkaMetrics -  - Failed to bind meter: kafka.consumer.fetch.manager.bytes.consumed.total [tag(client-id=dev-petl-app-sync-patient-skeletons-dfd9b010-3f87-43b9-9d2b-ca0352911812-StreamThread-1-consumer), tag(topic=dev-petl-app-sync-patient-skeletons), tag(kafka-version=2.5.1), tag(spring.id=streams.11)]. However, this could happen and might be restored in the next refresh.

As that error is quite unhelpful and wouldn't go away on a application refresh I decided to search further.

StackOverflow -- Spring Boot 2.3.2.RELEASE - Micrometer - KafkaMetrics - Failed to bind meter logs
- Had the same error but wasn't answered

So I searched this repo for "However, this could happen" I found the following lines:

https://github.com/micrometer-metrics/micrometer/blob/master/micrometer-core/src/main/java/io/micrometer/core/instrument/binder/kafka/KafkaMetrics.java#L195-L204

Knowing that we are using Prometheus I then went about break pointing our application at that if check and got the following:

Prometheus requires that all meters with the same name have the same set of tag keys. There is already an existing meter named 'kafka_consumer_fetch_manager_bytes_consumed_total' containing tag keys [client_id, env, instance, kafka_version, spring_id]. The meter you are attempting to register has keys [client_id, env, instance, kafka_version, spring_id, topic].

Being that the client-id was still the super long name that we don't care (because we want to group by topic name) I checked out our filter system which is copied as follows:

KafkaMetrics.kt
import io.micrometer.core.instrument.config.MeterFilter
import java.util.function.Function

/**
 * Contains utilities for enabling quality Kafka Metrics through Micrometer
 */
object KafkaMetrics {
    // tag names
    private const val CLIENT_ID = "client.id"
    private const val TOPIC_NAME = "topic.name"

    // <editor-fold desc="Topic Metrics">

    /**
     * By default Spring Cloud Streams Kafka has a tag of [CLIENT_ID]. For metrics dashboards
     * [TOPIC_NAME] is desired as it's constant across connector restarts and instances.
     * Therefor combine this method with [renameTopicNameValue] to create a constant [TOPIC_NAME].
     */
    fun renameClientIdToTopicName(): MeterFilter = MeterFilter
        .renameTag("kafka", CLIENT_ID, TOPIC_NAME)

    /**
     * Assuming [TOPIC_NAME] was added by [renameClientIdToTopicName] then note:
     *
     * By default Spring Cloud Streams Kafka has a tag of [CLIENT_ID] with a value of
     * roughly "topic + UUID + thread". These IDs change between connector restarts and instances.
     * Creating problems doing consistent metrics reporting overtime.
     *
     * This method shortens the value to just "topic" allowing constant metrics tags.
     * The recommendation for Spring Cloud Streams Kafka metrics is have a configuration of both
     * [renameClientIdToTopicName] and this filter to make a consistent [TOPIC_NAME] tag for your
     * connector.
     *
     * Example converts "dev-petl-app-sync-users-b231d016-ac0d-44d0-be8a-7d836119b06e-StreamThread-7-consumer"
     * to "dev-petl-app-sync-users".
     */
    fun renameTopicNameValue(): MeterFilter = MeterFilter.replaceTagValues(TOPIC_NAME, convertClientIdToTopicNameValue)

    private val CLIENT_ID_ENDING_REGEX = Regex("\\-\\w{8}\\-\\w{4}\\-\\w{4}\\-\\w{4}\\-\\w{12}\\-StreamThread\\-\\d+(\\-restore)?\\-consumer$")

    /**
     * @param input the client.id generated from Spring Cloud Streams
     *   (ex: "dev-petl-app-sync-users-b231d016-ac0d-44d0-be8a-7d836119b06e-StreamThread-7-consumer")
     * @return either the parsed topic "dev-petl-app-sync-users" in the case of the example input
     *   or the `input`
     */
    internal val convertClientIdToTopicNameValue: Function<String, String> = Function { input: String ->
        CLIENT_ID_ENDING_REGEX.find(input)
            ?.let {
                val topic = input.substring(0, it.range.first)
                if (it.groupValues[1].isEmpty()) topic else "$topic (restore)" // Is it a restore topic?
            }
            ?: input
    }

    // </editor-fold>
}

That's in a "common-lib" that is then included in the Spring application with:

import com.updox.petl.commons.metrics.KafkaMetrics
import io.micrometer.core.instrument.config.MeterFilter
import org.springframework.context.annotation.Bean
import org.springframework.context.annotation.Configuration

@Configuration
class MetricsConfiguration {
    @Bean
    fun renameClientIdToTopicName(): MeterFilter = KafkaMetrics.renameClientIdToTopicName()

    @Bean
    fun renameTopicNameValue(): MeterFilter = KafkaMetrics.renameTopicNameValue()
}

I noticed that client-id from the error didn't match the client.id that I was looking. Although, we knew that client.id worked in the past I decided to try the naive approach of updating the name to client-id which subsequently seemed to resolve the original error. e.g.:

object KafkaMetrics {
    // tag names
    private const val CLIENT_ID = "client-id"
    private const val TOPIC_NAME = "topic-name"

My question is was this changed in some release of Micrometer?
I want to make sure my system is resilient for future upgrades and doesn't break again with a mystery error that creates a lot of logs.

Micrometer v1.4.0 appears to have some big Kafka changes https://github.com/micrometer-metrics/micrometer/releases/tag/v1.4.0
- Specifically #1835 which does remove a number of client.id magic strings
Currently client-id appears hard coded in KafkaMetrics.java line 65 so maybe that's why? But the current test suite all looks for client.id as far as I can tell see kafka test dir for 1.5.4

@shakuzen
Copy link
Member

My question is was this changed in some release of Micrometer?
I want to make sure my system is resilient for future upgrades and doesn't break again with a mystery error that creates a lot of logs.

The JMX-based KafkaConsumerMetrics converts client-id (what Kafka registers) to client.id. With the newer KafkaMetrics class, we are not doing any conversion of the tag names and using them as-is from Kafka. This should explain the change from a client.id tag to a client-id tag. Now we have to figure out what to do about this inconsistency - switching back to the previous conversion to client.id would be another breaking change for those who have adapted to client-id.

@shakuzen shakuzen modified the milestones: 1.5.6, 1.6.0 Oct 23, 2020
@shakuzen shakuzen added the release notes Noteworthy change to call out in the release notes label Oct 28, 2020
shakuzen added a commit to shakuzen/micrometer that referenced this issue Oct 29, 2020
This also makes the `client.id` tag the same as it was in `KafkaConsumerMetrics`. Notably, `kafka-version` tag has also been changed to `kafka.version`. With some meter registries, this will effectively be no change (e.g. Prometheus), but for others it may be a breaking change to the tag name, but the goal is to correct the oversight of not aligning KafkaMetrics with KafkaConsumerMetrics initially. Behavior can be restored with a MeterFilter.

Resolves micrometer-metrics#2256
@shakuzen
Copy link
Member

#2316 will switches the tag naming back to what it was with KafkaConsumerMetrics, namely client.id (and kafka.version). It was an oversight that this wasn't aligned from the beginning. As demonstrated in the original comment of this issue, the behavior can be changed one way or the other with a MeterFilter.

Users who want client.id in 1.5.x should configure MeterFilter.renameTag("kafka", "client-id", "client.id") (also consider doing the same for kafka-version to kafka.version).
Users who want client-id in 1.6.x should configure the opposite filter: MeterFilter.renameTag("kafka", "client.id", "client-id").

Going forward we intend to stick with the canonical naming by default (client.id).

shakuzen added a commit that referenced this issue Oct 29, 2020
This also makes the `client.id` tag the same as it was in `KafkaConsumerMetrics`. Notably, `kafka-version` tag has also been changed to `kafka.version`. With some meter registries, this will effectively be no change (e.g. Prometheus), but for others it may be a breaking change to the tag name, but the goal is to correct the oversight of not aligning KafkaMetrics with KafkaConsumerMetrics initially. Behavior can be restored with a MeterFilter.

Resolves #2256
@3ygun
Copy link
Author

3ygun commented Oct 29, 2020

@shakuzen thanks and sounds good! My only request is if you can try to call out the change back in the 1.6 release notes? As I'll probably forget what you said by the time I go to upgrade next to 1.6? And, I imagine, Spring Boot 2.4ish?

@shakuzen
Copy link
Member

My only request is if you can try to call out the change back in the 1.6 release notes?

Absolutely plan on doing so (hence the release notes tag on the issue). I know this can be painful for affected users, which is why I waited to do the change until a minor release and will call it out in the release notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes Noteworthy change to call out in the release notes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants