search

micrometer-metrics / micrometer

An application observability facade for the most popular observability tools. Think SLF4J, but for observability.
https://micrometer.io
Apache License 2.0
4.39k stars 966 forks source link

Possible concurrency problems #5147

Open olga-larina opened 1 month ago

olga-larina commented 1 month ago

Hello. I have some questions about concurrency issues. We updated from Spring Boot 2.x to Spring Boot 3.1.4 and faced the problems with metrics. After investigating we found related issues in micrometer-metrics and spring-web (for example, https://github.com/micrometer-metrics/micrometer/issues/3874). Updating versions solved our problems.

But we found several places, where concurrency problems can arise. Maybe we are wrong and these lines can be accessed only from single thread. Can you please have a look at them?

Thanks!

https://github.com/micrometer-metrics/micrometer/blob/23b6c43d9ce7a758dd5aa4620c776358f3a86039/micrometer-observation/src/main/java/io/micrometer/observation/SimpleObservation.java#L49 - volatile ObservationConvention convention ?

https://github.com/micrometer-metrics/micrometer/blob/23b6c43d9ce7a758dd5aa4620c776358f3a86039/micrometer-observation/src/main/java/io/micrometer/observation/SimpleObservation.java#L95 - ConcurrentLinkedDeque instead of ArrayDeque?

https://github.com/micrometer-metrics/micrometer/blob/23b6c43d9ce7a758dd5aa4620c776358f3a86039/micrometer-observation/src/main/java/io/micrometer/observation/Observation.java#L913-L930 - volatile name, contextualName, error, parentObservation? ConcurrentHashMap lowCardinalityKeyValues and highCardinalityKeyValues?

https://github.com/micrometer-metrics/micrometer/blob/23b6c43d9ce7a758dd5aa4620c776358f3a86039/micrometer-observation/src/main/java/io/micrometer/observation/transport/RequestReplyReceiverContext.java#L33 - volatile RES response?

https://github.com/micrometer-metrics/micrometer/blob/23b6c43d9ce7a758dd5aa4620c776358f3a86039/micrometer-observation/src/main/java/io/micrometer/observation/transport/RequestReplySenderContext.java#L33 - volatile RES response?

https://github.com/micrometer-metrics/micrometer/blob/23b6c43d9ce7a758dd5aa4620c776358f3a86039/micrometer-observation/src/main/java/io/micrometer/observation/transport/ReceiverContext.java#L37-L44 - volatile carrier, remoteServiceName, remoteServiceAddress?

https://github.com/micrometer-metrics/micrometer/blob/23b6c43d9ce7a758dd5aa4620c776358f3a86039/micrometer-observation/src/main/java/io/micrometer/observation/transport/SenderContext.java#L37-L44 - volatile carrier, remoteServiceName, remoteServiceAddress?

olga-larina commented 1 month ago

And I have a question about LongTaskTimer (xxx.active metrics). Should I create another issue for it?

https://github.com/micrometer-metrics/micrometer/blob/23b6c43d9ce7a758dd5aa4620c776358f3a86039/micrometer-core/src/main/java/io/micrometer/core/instrument/observation/DefaultMeterObservationHandler.java#L72-L79

LongTaskTimer is created on start of the observation with tags. But at this time in Spring we have only default tag values (for example, http status code). Is it a problem in Spring web?

olga-larina commented 1 month ago

And I have a question about LongTaskTimer (xxx.active metrics). Should I create another issue for it?

https://github.com/micrometer-metrics/micrometer/blob/23b6c43d9ce7a758dd5aa4620c776358f3a86039/micrometer-core/src/main/java/io/micrometer/core/instrument/observation/DefaultMeterObservationHandler.java#L72-L79

LongTaskTimer is created on start of the observation with tags. But at this time in Spring we have only default tag values (for example, http status code). Is it a problem in Spring web?

This question was answered in https://github.com/spring-projects/spring-framework/issues/32897

jonatan-ivanov commented 1 week ago

Thank you for the issue. In general Observations should not be modified from multiple threads concurrently so I'm curious what you are trying to do.

volatile ObservationConvention convention ?

Why should this be volatile? The convention is set from the ctor and a setter, this should not be thread safe I think, if you have concurrency issues with it, could you please tell us what you are trying to do (a minimal java test/reproducer would help)?

ConcurrentLinkedDeque instead of ArrayDeque?

Why should this be concurrent? The deque here is a local variable, cannot be accessed from multiple threads

Everything else: I think they are the same as the convention, I don't think you should access them concurrently so they don't ned to be thread safe. If you have concurrency issues with them, could you please tell us what you are trying to do (a minimal java test/reproducer would help)?

github-actions[bot] commented 4 days ago

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.