Add childContext to MetricContext #30
Conversation
| @Override | ||
| public MetricContext newChildContext(TypedMap attributes) { | ||
| ImmutableTypedMap.Builder childAttributes = ImmutableTypedMap.Builder.from(this.attributes); | ||
| attributes.stream().forEach(entry -> childAttributes.add(entry.getKey(), entry.getValue())); |
There was a problem hiding this comment.
I prefer using a parent pointer here for a few reasons.
I think Metrics Contexts are hierarchical, as such I think the structure should reflect that.
Copying all these values into a new map at this point is slow for the running application.
There's potential for naming collisions, potentially stoping valuable context information from the parent.
Copying all the values defines how contexts are aggregated here, not in the recorder where that aggregation belongs.
There was a problem hiding this comment.
On the other hand, if children are intended to be completely independent, you could end up with extra context objects hanging around just because childs still refer to them... Think the whole parent/child relationship needs to be codified better before choosing one strategy over the other.
| /** | ||
| * Create a child context with the supplied attributes. | ||
| * The child context will inherited the attributes from this instance and | ||
| * use the supplied attributes as overrides. |
There was a problem hiding this comment.
This would probably be better phrased as something like "The child context will contain the provided attributes as well as inherit the attributes from this instance. Where the provided attributes conflict, they will override the values from this parent.
Also: Can you be explicit about the relationship between parent/child here? Is the child still related to the parent in any way, or are they completely separate things once created? This needs to be defined at the interface level, or else different implementations may make different, incompatible choices.
| * @param attributes the attribute overrides to apply. | ||
| * @return a new child context with combined attributes. | ||
| */ | ||
| MetricContext newChildContext(TypedMap attributes); |
There was a problem hiding this comment.
'new' here is not really necessary, and 'context' is somewhat redundant - why not just call this 'child' or similar?
| MetricContext newChildContext(TypedMap attributes); | ||
|
|
||
| /** | ||
| * Create a new child context with attributes attributes inherited from this instance. |
There was a problem hiding this comment.
What good is this method?
If the new child has the same attributes as the existing context, what makes it a different context? Context are defined by the metadata comprising them (aka 'attributes'), doesn't make a lot of sense to have two logically distinct contexts which are functionally the same.
| @@ -13,7 +13,6 @@ | |||
| * <p/> | |||
| * A {MetricContext} is the primary interface used to record measurements. | |||
| */ | |||
| // TODO: provide a childContext() mechanism? | |||
| public interface MetricContext extends AutoCloseable { | |||
There was a problem hiding this comment.
I probably missed the boat on this one, but:
I'm not sure calling this a 'MetricContext' is strictly the best idea. It is conceptually a context in the larger sense, that only happens to be very useful for the metrics stuff here - one of the design intents was for it to be a more widely usable context (for things such as request tracking etc.) that just happened to be needed/used for metrics first.
| @Override | ||
| public MetricContext newChildContext(TypedMap attributes) { | ||
| ImmutableTypedMap.Builder childAttributes = ImmutableTypedMap.Builder.from(this.attributes); | ||
| attributes.stream().forEach(entry -> childAttributes.add(entry.getKey(), entry.getValue())); |
There was a problem hiding this comment.
On the other hand, if children are intended to be completely independent, you could end up with extra context objects hanging around just because childs still refer to them... Think the whole parent/child relationship needs to be codified better before choosing one strategy over the other.
|
|
||
| import java.time.Instant; | ||
|
|
||
| final class DefaultMetricContext implements MetricContext { |
There was a problem hiding this comment.
Javadoc?
Why does this class exist, etc. etc. For example: why is this called 'Default' when it's also final? When would one ever use a different implementation?
|
|
||
| public class DefaultMetricContextTest { | ||
| @Test | ||
| public void childContextReturnsCombinedAttributes() { |
| MetricContext parentContext = grandParentContext.newChildContext(parentAttributes); | ||
| MetricContext childContext = parentContext.newChildContext(childAttributes); | ||
|
|
||
| TypedMap actualChildAttributes = childContext.attributes(); |
There was a problem hiding this comment.
While you're at it maybe assert the immutableness of the parents
Add the ability to create a child MetricContext from an instance. The child context will inherit the attributes from the parent with the option of providing additional attributes as overrides.
The child and parent lifecycles are independent. Calling close on one of them will not effect the other.