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.

Sign up for GitHub

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

Append missing _total suffix when registering Counter metrics. #177

Merged
merged 5 commits into from
Sep 13, 2022
Merged

Append missing _total suffix when registering Counter metrics. #177

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
930aec4
Append missing _total suffix when registering Counter metrics.
alexcsf Aug 26, 2022
7b24edf
fix: MediaType annotation for application/json.
alexcsf Aug 26, 2022
561d38b
log: Change log level to DEBUG.
alexcsf Aug 31, 2022
d46e1fe
test: Add tests to cover counter "_total" suffix.
alexcsf Aug 31, 2022
883a4f6
Add check for base metric name when registering a counter.
alexcsf Aug 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions jicoco-metrics/src/main/kotlin/org/jitsi/metrics/MetricsContainer.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ package org.jitsi.metrics

import io.prometheus.client.CollectorRegistry
import io.prometheus.client.exporter.common.TextFormat
import org.jitsi.utils.logging2.Logger
import org.jitsi.utils.logging2.LoggerImpl
import org.json.simple.JSONObject
import java.io.IOException
import java.io.StringWriter
Expand All @@ -28,6 +30,7 @@ open class MetricsContainer @JvmOverloads constructor(
/** the registry used to register metrics */
val registry: CollectorRegistry = CollectorRegistry.defaultRegistry
) {
private val logger: Logger = LoggerImpl(javaClass.name)
Copy link
Member

Choose a reason for hiding this comment

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

Use createLogger()


/**
* Namespace prefix added to all metrics.
Expand Down Expand Up @@ -110,17 +113,23 @@ open class MetricsContainer @JvmOverloads constructor(
/** the optional initial value of the metric */
initialValue: Long = 0
): CounterMetric {
if (metrics.containsKey(name)) {
val newName = when (name.endsWith("_total")) {
Copy link
Member

Choose a reason for hiding this comment

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

Why use "when" instead of "if"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I was playing around and forgot to revert it to "if". Should I go ahead and change it to this?

val newName = if (name.endsWith("_total")) name else "${name}_total".also {
    logger.warn("Counter '$name' was renamed to '$it' to ensure consistent metric naming.")
}

Also, it currently logs whenever a metric is renamed (which currently applies to all of our CounterMetric). I think it's better to log this behavior even though it's the most common, as opposed to logging whenever a metric name is unchanged.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. But make it INFO or DEBUG

true -> name
false -> "${name}_total".also {
logger.warn("Counter '$name' was renamed to '$it' to ensure consistent metric naming.")
}
}
if (metrics.containsKey(newName)) {
Copy link
Member

Choose a reason for hiding this comment

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

If there is another metric with "name" the Prometheus lib will throw, right? What do you think about handling that case here, so it's clear that it's expected and the warning message is clear

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it throws an IllegalArgumentException. So were you thinking of trying to return a newly-registered metric, and catch the exception and deal with it?

Copy link
Member

Choose a reason for hiding this comment

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

No, I was thinking of catching the case and throwing our own exception. Just a suggestion I'm not sure this is a good idea or worth doing.

if (checkForNameConflicts) {
throw RuntimeException("Could not register metric '$name'. A metric with that name already exists.")
throw RuntimeException("Could not register metric '$newName'. A metric with that name already exists.")
}
return metrics[name] as CounterMetric
return metrics[newName] as CounterMetric
}
return CounterMetric(name, help, namespace, initialValue).apply { metrics[name] = register(registry) }
return CounterMetric(newName, help, namespace, initialValue).apply { metrics[newName] = register(registry) }
}

/**
* Creates and registers a [CounterMetric] with the given [name], [help] string and optional [initialValue].
* Creates and registers a [LongGaugeMetric] with the given [name], [help] string and optional [initialValue].
*
* Throws an exception if a metric with the same name but a different type exists.
*/
Expand Down