Skip to content
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

TaggedMetricRegistry composability #81

Merged
merged 9 commits into from
Jun 26, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
import com.codahale.metrics.Metric;
import com.codahale.metrics.Timer;
import com.google.auto.service.AutoService;
import java.util.Collections;
import com.google.common.collect.ImmutableMap;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.ConcurrentHashMap;
Expand All @@ -36,6 +36,7 @@ public final class DefaultTaggedMetricRegistry implements TaggedMetricRegistry {
private static final TaggedMetricRegistry DEFAULT = new DefaultTaggedMetricRegistry();

private final Map<MetricName, Metric> registry = new ConcurrentHashMap<>();
private final Map<Map<String, String>, TaggedMetricSet> taggedRegistries = new ConcurrentHashMap<>();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably not quite right, since will want to shove probably two MetricSets in for each AtlasDB, also probably shouldn't be passing in an empty map here. Maybe just a Set of Pair type structure is the right thing here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this feels odd and I worry about someone putting a mutable Map<String, String> in as a key and causing all kinds of chaos.


public DefaultTaggedMetricRegistry() {}

Expand Down Expand Up @@ -93,14 +94,34 @@ public Timer timer(MetricName metricName, Supplier<Timer> timerSupplier) {

@Override
public Map<MetricName, Metric> getMetrics() {
return Collections.unmodifiableMap(registry);
ImmutableMap.Builder<MetricName, Metric> result = ImmutableMap.<MetricName, Metric>builder()
.putAll(registry);
taggedRegistries.forEach((tags, metrics) -> metrics.getMetrics()
.forEach((metricName, metric) -> result.put(
MetricName.builder()
.from(metricName)
.putAllSafeTags(tags)
.build(),
metric)));

return result.build();
}

@Override
public Optional<Metric> remove(MetricName metricName) {
return Optional.ofNullable(registry.remove(metricName));
}

@Override
public void addMetrics(Map<String, String> tags, TaggedMetricSet other) {
taggedRegistries.put(tags, other);
}

@Override
public void removeMetrics(Map<String, String> tags) {
taggedRegistries.remove(tags);
}

private <T extends Metric> T getOrAdd(MetricName metricName, Class<T> metricClass, Supplier<T> metricSupplier) {
Metric metric = registry.computeIfAbsent(metricName, name -> metricSupplier.get());
if (!metricClass.isInstance(metric)) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* Copyright 2018 Palantir Technologies, Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.palantir.tritium.metrics.registry;

import static java.util.stream.Collectors.toMap;

import com.codahale.metrics.Metric;
import com.codahale.metrics.MetricRegistry;
import java.util.Map;

public final class DropwizardTaggedMetricSet implements TaggedMetricSet {
private final MetricRegistry metricRegistry;

public DropwizardTaggedMetricSet(MetricRegistry metricRegistry) {
this.metricRegistry = metricRegistry;
}

@Override
public Map<MetricName, Metric> getMetrics() {
return metricRegistry.getMetrics().entrySet().stream()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could avoid the copy here if we think it's important by implementing ForwardingMap.

.collect(toMap(entry -> MetricName.builder().safeName(entry.getKey()).build(), Map.Entry::getValue));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we ok to assume all metric names are safe here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes - they'll get logged internally in the same way as they currently do. The reason this was called safe name was to make clear that they have to be safe - but that's a baseline requirement.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
/**
* Similar to {@link com.codahale.metrics.MetricRegistry} but allows tagging of {@link Metric}s.
*/
public interface TaggedMetricRegistry {
public interface TaggedMetricRegistry extends TaggedMetricSet {

/**
* Returns existing or new timer metric for the specified metric name.
Expand Down Expand Up @@ -81,13 +81,6 @@ public interface TaggedMetricRegistry {

Counter counter(MetricName metricName, Supplier<Counter> counterSupplier);

/**
* Returns a map of registered metrics.
*
* @return map of registered metrics
*/
Map<MetricName, Metric> getMetrics();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked at how painful the API break would be, do you have a sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's not actually an API break; it's just moved into the super interface


/**
* Removes the tagged metric with the specified metric name.
*
Expand All @@ -96,4 +89,20 @@ public interface TaggedMetricRegistry {
*/
Optional<Metric> remove(MetricName metricName);

/**
* Adds a set of metrics to this TaggedMetricRegistry's metric set,
* which are to be uniquely identified by the tags provided.
* <p>
* So, if I have a metric registry with a single metric called 'foo', and I add it
* with tag (bar, baz), this registry will now contain 'foo', tagged with (bar, baz).
*
* @param tags the tags which will be added to all the metrics in the metric set
* @param metrics the metrics which should be added
*/
void addMetrics(Map<String, String> tags, TaggedMetricSet metrics);

/**
* Removes a TaggedMetricsSet added via addMetrics from this metrics set.
*/
void removeMetrics(Map<String, String> tags);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* Copyright 2018 Palantir Technologies, Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.palantir.tritium.metrics.registry;

import com.codahale.metrics.Metric;
import java.util.Map;

public interface TaggedMetricSet {
/**
* Returns a map of metrics.
*
* @return map of metrics
*/
Map<MetricName, Metric> getMetrics();
}