Full POC for metrics system #1844
Conversation
dagnir
force-pushed
the
dongie-metrics-poc
branch
from
7ddad4a to
25d234f
Compare
core/metrics-spi/src/main/java/software/amazon/awssdk/metrics/MetricCollection.java
Show resolved
Hide resolved
| /** | ||
| * An immutable collection of metrics. | ||
| */ | ||
| public interface MetricCollection extends Iterable<MetricRecord<?>> { |
| * A specific SDK metric | ||
| * @param <T> | ||
| */ | ||
| public final class SdkMetric<T> { |
There was a problem hiding this comment.
Any way we can reuse some of the logic from AttributeMap.Key for this?
There was a problem hiding this comment.
Yep, these are modeled after AttributeMap.Key, so it should be possible
| * @param <T> | ||
| */ | ||
| public final class SdkMetric<T> { | ||
| private static final Set<SdkMetric<?>> SDK_METRICS = new HashSet<>(); |
| @Override | ||
| public void close() { | ||
| if (closed) return; | ||
|
|
||
| children.forEach(MetricCollector::close); | ||
|
|
||
| this.closed = true; | ||
| } |
There was a problem hiding this comment.
This doesn't really do anything, except prevent publish from getting called, right? Do we really need this?
There was a problem hiding this comment.
Yeah, mostly premature optimization. I wanted to prevent cases where the whole tree is traversed unnecessarily.
core/metrics-spi/src/main/java/software/amazon/awssdk/metrics/SdkMetric.java
Show resolved
Hide resolved
| public DefaultMetricCollection(String name, Map<SdkMetric<?>, List<MetricRecord<?>>> metrics, Collection<MetricCollection> children) { | ||
| this.name = name; | ||
| this.metrics = metrics; | ||
| this.children = children != null ? Collections.unmodifiableCollection(children) : Collections.emptyList(); | ||
| } |
There was a problem hiding this comment.
We should be clear in the Javadoc that we're not doing copies of these inputs, since in other places we do that.
| @Override | ||
| public Collection<MetricCollection> children() { | ||
| return children; | ||
| } |
There was a problem hiding this comment.
Can we do a List, so that it's ordered?
There was a problem hiding this comment.
Sure, I think that could be valuable
| /** | ||
| * This is an umbrella category (provided for convenience) that records metrics belonging to every category | ||
| * defined in this enum. Clients who wish to collect lot of SDK metrics data should use this. | ||
| * <p> | ||
| * Note: Enabling this option is verbose and can be expensive based on the platform the metrics are uploaded to. | ||
| * Please make sure you need all this data before using this category. | ||
| */ | ||
| ALL("all") |
There was a problem hiding this comment.
It feels like we have to treat "all" magically. Can't we just do MetricCategory.values() to get all of them?
ignoring kinesis test that fails during release
POC of the metrics system for V2 incorporating the feedback from @bmaizels and @millems. Includes the metrics SPI, default implementations of these classes, and wiring up the metrics into an API call (for XML protocols).