Skip to content

Conversation

@jacomago
Copy link
Contributor

They take some resources, so if your are querying a lot of properties this can timeout

@jacomago jacomago self-assigned this May 21, 2025
@simon-ess
Copy link

This seems like a nice addition. However, if the 5 second interval actually a good one? It seems that switching to re-running this that often might just be hammering the service in a different way.

@jacomago
Copy link
Contributor Author

This seems like a nice addition. However, if the 5 second interval actually a good one? It seems that switching to re-running this that often might just be hammering the service in a different way.

Its configurable.

@anderslindho
Copy link
Contributor

5s default seems a bit overkill to me - is there any use case for that time precision?

@simon-ess
Copy link

Would it not be possible to update the atomic count every time a tag or a property is added or removed? Or is that perhaps a fool's errand?

(The point being of course that the number of tags or proprties will not change in the last $INTERVAL unless something has actually changed like someone manually adding a tag or some such)

@jacomago
Copy link
Contributor Author

Would it not be possible to update the atomic count every time a tag or a property is added or removed? Or is that perhaps a fool's errand?

(The point being of course that the number of tags or proprties will not change in the last $INTERVAL unless something has actually changed like someone manually adding a tag or some such)

That is a separate metric, that isn't changed in this PR and it is tracked. CF_PROPERTY_COUNT and CF_TAG_COUNT in MetricsService.

They take some resources, so if your are querying a lot of properties this can timeout
@jacomago jacomago force-pushed the addtimeronmetricsagain branch from 9e2a6c7 to ab12ccb Compare May 21, 2025 09:03
@jacomago
Copy link
Contributor Author

5s default seems a bit overkill to me - is there any use case for that time precision?

Updated to 10 seconds

@anderslindho
Copy link
Contributor

Prometheus default scrape interval is 1m so we could do that or 30s to save CF resources 🔧

But otherwise LGTM

@simon-ess
Copy link

One other question: Does this actually have to be an AtomicLong? As near as I see it, we update these values every $INTERVAL seconds in a single place. What is the need for concurrency protection?

(I mean, other than if the update itself takes longer than the interval... 😄 )

@sonarqubecloud
Copy link

@jacomago
Copy link
Contributor Author

One other question: Does this actually have to be an AtomicLong? As near as I see it, we update these values every $INTERVAL seconds in a single place. What is the need for concurrency protection?

(I mean, other than if the update itself takes longer than the interval... 😄 )

I'm not sure if necessary when using the GaugeBuilder, but generally using Atomics is suggested in the documentation https://docs.micrometer.io/micrometer/reference/concepts/gauges.html

@jacomago jacomago merged commit 7e53182 into ChannelFinder:master May 27, 2025
6 checks passed
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