-
Notifications
You must be signed in to change notification settings - Fork 37
putMetric in MetricsLogger is not ThreadSafe and leading to ArrayIndexOutOfBoundsExceptions #61
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
Comments
The library is not thread-safe. Your fix can fix one place but there're also other places needs. We will prioritize the enhancement of this. |
We also need to make sure that the size of the list does not go beyond the current 100 limit. |
The current API is not compatible with being used in a multi-threaded application at all. Since Are there any plans to address this? |
@goldmar this assumes you’re using the same metrics logger instance on multiple threads, right? Can you use a different instance per thread? You could access it through a ThreadLocal to simplify this. There are definitely other enhancements we need to make to improve throughput for socket writing but even with that I think you’d still want a distinct logger per thread. Can you help us understand your use case a little better? It’s possible I’ve misunderstood. Is there some issue you’re hitting where distinct metrics loggers per thread are not working as expected? |
@jaredcnance Thank you for the quick reply! We have started building our first service on ECS Fargate and are just trying to figure out the best way to emit metrics. We have configured Firelens with fluentbit and it works well, however I am not sure yet what is the best way to use this logging library. We have an Akka Streams application which consumes data from Kinesis and SQS and writes to Elasticsearch. I don't think using ThreadLocals is the way to go in an async context; I think I'll create a custom Sink for emitting metrics in Akka instead. This will guarantee that there is only a single thread calling the |
I see, so you want to be able to do something like:
Or even:
Is that the generalized use case? |
So what I would like to do, is pass around a single MetricsLogger around in my async contexts and emit my metrics. This is obviously not possible because MetricsLogger is not only not threadsafe but also has the above mentioned limitations of its API. If you look at the implementations of different Since this is currently not possible, a workaround is required. It could be using a |
We discussed offline and to restate goldmar@'s specific ask is for an additional interface that simplifies the emission of metrics where add(
String key,
Double value,
CloudWatchUnit unit,
List<DimensionSet> dimensions,
Map<String, String> properties
);
addAll(
List<Metric> metrics,
List<DimensionSet> dimensions,
Map<String, String> properties
); |
@dbadami Hi Devavrat, |
Hi,
We're adding metrics using using the MetricsLogger
putMetric
method in our application. We have multiple threads adding a metric for the same key and we received an ArrayIndexOutOfBoundsException when doing so. In our logs we were able to track it down to the addValue method inMetricDefinition
.Our hypothesis is that this is happening because the resizing of the array list is synchronized on so you can have an element that can being inserted at index that is greater than the current size of the array.
aws-embedded-metrics-java/src/main/java/software/amazon/cloudwatchlogs/emf/model/MetricDefinition.java
Line 61 in 9875fed
Happy to make the fix if the change is as simple as:
Thanks,
Devavrat
The text was updated successfully, but these errors were encountered: