-
Notifications
You must be signed in to change notification settings - Fork 97
feat(metrics): introduce Metrics.flushMetrics #2154
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
base: main
Are you sure you want to change the base?
Conversation
- introduce `Metrics.flushMetrics` as a more powerful version of `flushSingleMetrics` to allow - using defaults by inheriting state e.g. namespace, dimensions and metadata - emitting multiple metrics in one metrics context - refactor `flushSingleMetrics` to use `flushMetrics` - move namespace/service setting from `MetricsFactory` to `EmfMetricsLogger`
6587c5a
to
13eb9d1
Compare
- introduce `Metrics.flushMetrics` as a more powerful version of `flushSingleMetrics` to allow - using defaults by inheriting state e.g. namespace, dimensions and metadata - emitting multiple metrics in one metrics context - refactor `flushSingleMetrics` to use `flushMetrics` - move namespace/service setting from `MetricsFactory` to `EmfMetricsLogger`
13eb9d1
to
537dc90
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @humanzz for sending this PR. Overall, it looks good and I left some comments that we need to address before moving forward.
Primarily, can you go back to the issue #2153 and let us know your use-case? We still need to decide whether we want to move forward with inheriting the default configuration of the metrics logger for flushMetrics
and flushSingleMetric
.
|
||
// Apply default configuration from environment variables | ||
String envNamespace = System.getenv("POWERTOOLS_METRICS_NAMESPACE"); | ||
if (envNamespace != null) { | ||
metrics.setNamespace(envNamespace); | ||
} | ||
|
||
// Only set Service dimension if it's not the default undefined value | ||
String serviceName = LambdaHandlerProcessor.serviceName(); | ||
if (!LambdaConstants.SERVICE_UNDEFINED.equals(serviceName)) { | ||
metrics.setDefaultDimensions(DimensionSet.of("Service", serviceName)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see in your PR description you moved this logic to EmfMetricsLogger
to reduce the number of places that can set the namespace. I don't see how this reduces the number of places. In fact, it will increase the number of places if we add another metrics backend in addition to the EMF metrics logger.
Let's keep this in MetricsFactory
. The idea of this architecture is that EmfMetricsLogger
is a pluggable metrics backend that is only to be instantiated with an initial configuration by MetricsFactory
or MetricsBuilder
. This is also why it is in the internal
package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did get the idea behind MetricsFactory
and pluggable backends and I had some initial impressions that it might benefit from a bit more tightening. Now I had a look again at the factory and the builder, I think you're more right but let me share a couple of thoughts
- Initially, I thought that
EmfMetricsLogger
should use either theMetricsProvider
orMetricsFactory
to initialize a new instance ofMetrics
so it gets whichever default configurations (in this case environment variable namespace logic and the service name) which then contributed to moving more of the logic into theEmfMetricsLogger
- The other motivation, is that I see properties getting set on Metrics, spread across
LambdaMetricsAspect
andEmfMetricsLogger
andMetricsFactory
MetricsFactory
is a bit odd in that it allows setting/overriding theMetricsProvider
which can happen at any point in the lifetime of the class which can lead to issues if any piece of code decides to override the provider and other pieces rely ongetMetricsInstance
assuming it's all the global metrics instance that has been configured- The fact that one now creates a
Metrics
instance in the handler (https://docs.powertools.aws.dev/lambda/java/latest/core/metrics/#creating-metrics) via theMetricsFactory
orMetricsBuilder
which would still under the hood setup/use theMetricsFactory
where other non-handler code, if they want to use that instance would rely onMetricsFactory.getMetricsInstance()
and hope/assume it's the same one defined in the lambda handler - whilesetMetricsProvider
leaves the door open to overriding the provider is a bit problematic
I know some of the points above are not necessarily directly related to this PR but the thoughts came as I was trying to work on this.
I will return this piece to the MetricsFactory
, and maybe for later, we can think about the points raised above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this feedback about the implementation design. Let me explain some thoughts that went into it:
- The idea for
MetricsFactory
is to return a well configured (opinionated)Metrics
backend Singleton. This should work independent ofLambdaMetricsAspect
i.e. independent of forcing the user to use AspectJ - For users not using AspectJ, the
MetricsBuilder
can be used as an easy way to set the configuration on a default initializedMetrics
backend returned byMetricsFactory
- For users using AspectJ with the
@FlushMetrics
annotation, theLambdaMetricsAspect
only applies the configuration that is specific to the Aspect.
All of this is designed in a way that is agnostic to the metrics backend and to achieve a consistent configuration precedence (https://docs.powertools.aws.dev/lambda/java/latest/core/metrics/#order-of-precedence-of-metrics-configuration). So, it is actually intentional that these different components all set configuration on the Metrics
backend because the user is given different ways of configuring the metrics backend (Env vars, builder pattern, aspectj).
The idea of having the setMetricsProvider
method was to allow users to:
- Set another metrics backend programatically
- Bring their own metrics provider implementation
Both of these scenarios are not documented yet.
I agree with your point that exposing this as a public method is not 100% ideal. Do you have a suggestion for a better way of achieving this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of potential ideas
- it's kept public, but it can be only set once, and trying to override it throws an exception
- Using an approach like SPI to select the provider and completely dropping the ability to configure the provider via code - either for
MetricsFactory
orMetricsBuilder
- Only allow
MetricsBuilder
to configure the provider, and it sets it onMetricsFactory
usingsetMetricsProvider
but make it package-private... but this still suffers from the issue that if someone initializes anotherMetrics
usingMetricsBuilder
they might override the provider on the factory again - but we can prevent the overrides in a way similar to 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like these ideas. We use SPIs for e.g. idempotency or logging. In the current monolithic module setup for metrics I am not a fan of SPI though. Usually, we provide modules that auto-register. For example: powertools-logging
will be used with powertools-logging-log4j2
or powertools-logging-logback
.
Introducing SPIs will be a major version update and I will consider this when adding additional larger metrics backend. Great idea!
I really like option 1. It is simple enough and makes a lot of sense for such singleton classes – we don't want people to accidentally propagate a different metrics provider throughout the code base outside of initial configuration.
Would you be up for adding this in this PR including a small unit test?
...trics/src/main/java/software/amazon/lambda/powertools/metrics/internal/EmfMetricsLogger.java
Outdated
Show resolved
Hide resolved
...trics/src/main/java/software/amazon/lambda/powertools/metrics/internal/EmfMetricsLogger.java
Outdated
Show resolved
Hide resolved
if (namespace != null) { | ||
metrics.setNamespace(this.namespace); | ||
} | ||
defaultDimensions.forEach(metrics::addDimension); | ||
metadata.forEach(metrics::addMetadata); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We haven't decided yet in the issue if we want to auto-inherit the default metrics logger configuration. I see in v1 this is the behavior we have.
I've addressed the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick turnaround @humanzz. Just one more thing that I discovered.
...trics/src/main/java/software/amazon/lambda/powertools/metrics/internal/EmfMetricsLogger.java
Outdated
Show resolved
Hide resolved
...trics/src/main/java/software/amazon/lambda/powertools/metrics/internal/EmfMetricsLogger.java
Outdated
Show resolved
Hide resolved
...s/src/test/java/software/amazon/lambda/powertools/metrics/internal/EmfMetricsLoggerTest.java
Show resolved
Hide resolved
Good catch! It seems that a "property" is simply a key added to the log line while "metadata" becomes part of the Cold start metric (putProperty) {
"_aws": {
"Timestamp": 1751977621635,
"CloudWatchMetrics": [
{
"Namespace": "ServerlessAirline",
"Metrics": [
{
"Name": "ColdStart",
"Unit": "Count"
}
],
"Dimensions": [
[
"Service",
"FunctionName"
]
]
}
]
},
"function_request_id": "b8628984-b745-44bb-8025-247c2da76da1",
"xray_trace_id": "1-686d0e92-396456d519ba8c28174120a4",
"ColdStart": 1,
"Service": "payment",
} Regular metric (putMetadata) {
"_aws": {
"Timestamp": 1751977946689,
"CloudWatchMetrics": [
{
"Namespace": "ServerlessAirline",
"Metrics": [
{
"Name": "CustomMetric1",
"Unit": "Count"
},
{
"Name": "CustomMetric3",
"Unit": "Count",
"StorageResolution": 1
}
],
"Dimensions": [
[
"Service"
]
]
}
],
"function_request_id": "feccb848-47a9-4b32-a16b-73d45d7ad308",
"xray_trace_id": "1-686d0fdb-651f4fc05b57221e726890c0"
},
"CustomMetric1": 1,
"Service": "payment",
"CustomMetric3": 1
} I need to find out the semantic difference – those nuances don't exist in the other runtimes. None of it is searchable in cloudwatch metrics. Besides the location in the json output I don't see any difference. Update: It appears that putMetadata will add key-value pairs to the We should update the |
|
||
@Override | ||
public void addMetadata(String key, Object value) { | ||
emfLogger.putMetadata(key, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following this comment we should not call putMetadata
but putProperty
. It is the documented way and consistent with the other language runtimes. This will make the usage consistent again.
Thanks for catching this detail!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Was about the write this but you bet me to it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thought though, Metrics.addMetada
and the new Metrics.addProperty
which I'll introduce, are likely EMF-only features so there's some bleeding of the backend metrics engine to the abstraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we misunderstood each other. My suggestion was not add addProperty
but instead replace the code within addMetadata
with a call to putProperty
. In the other Powertools runtimes we treat "metadata" as what is called in the Java EMF library "putProperty".
Users should not call addMetadata on the EMF library at all.
I also agree with your comment about bleeding. It is not possible to workaround this anymore without introducing a breaking change – in other backends this method will likely not have any effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed the part about using putProperty
in addMetadata
change coming!
...trics/src/main/java/software/amazon/lambda/powertools/metrics/internal/EmfMetricsLogger.java
Outdated
Show resolved
Hide resolved
|
Summary
Changes
Metrics.flushMetrics
as a more powerful version offlushSingleMetrics
to allowEmfMetricsLogger.addMetadata
to use emf'sputProperty
flushSingleMetrics
andcaptureColdStartMetric
to useflushMetrics
MetricsUtils
Issue number: #2153
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.