Timer StopWatch should allow adding tags after timer has been started

[tommack]

This would allow timers to be used in a more flexible manner.

try/catch example:

TaggableStopWatch stopwatch = someTime.start();
try {
    // do stuff
    stopwatch.addTag("result", "success");
} catch (...) {
    stopwatch.addTag("result", "exception");
} finally {
    stopwatch.stop();
}

classification example:

TaggableStopWatch stopwatch = someTime.start();
// make some call to determine more about this task
stopwatch.addTag("taskType", "some value returned above");
// do some more work
stopwatch.stop();

simple chaining example (not compelling on its own):

TaggableStopWatch stopwatch = someTime.start().addTag("someTag", "someValue");
// do stuff
stopwatch.addTag("otherTag", "otherValue").stop();

A rough implementation would look like (important stuff in the stop() method):

public class SomeTimerClass { // this could be written in a static style like DynamicTimer as well
    private final MonitorConfig config;
    private LoadingCache<MonitorConfig, Timer> timerCache = ...; // Guava cache, or maybe can delegate to DynamicTimer

    // standard methods to setup config, same pattern as BasicTimer

    public TaggableStopwatch start() {
        return new TaggableStopwatch() {
            private final long startTime = System.nanoTime();
            private volatile long stopTime = -1;
            private volatile BasicTagList tagList = BasicTagList.of(ImmutableMap.<String, String>of());

            @Override
            public TaggableStopwatch addTags(Tag firstTag, Tag... otherTags) {
                checkState();
                List<Tag> asList = Lists.asList(firstTag, otherTags);
                BasicTagList newList = BasicTagList.of(asList.toArray(new Tag[0]));
                return addTags(newList);
            }

            @Override
            public TaggableStopwatch addTags(String tagName, String tagValue, String... otherTags) {
                checkState();
                List<String> asList = Lists.asList(tagName, tagValue, otherTags);
                BasicTagList newList = BasicTagList.of(asList.toArray(new String[0]));
                return addTags(newList);
            }

            @Override
            public TaggableStopwatch addTags(TagList newList) {
                checkState();
                tagList = tagList.copy(newList);
                return this;
            }

            @Override
            public void stop() {
                checkState();
                stopTime = System.nanoTime();
                Timer timer = getDynamicTimer(tagList); // loads a Timer from a cache like DynamicTimer
                timer.record(getDuration(), TimeUnit.NANOSECONDS);
            }

            private void checkState() {
                if (stopTime == -1) return;
                throw new IllegalStateException("Already stopped");
            }

            @Override
            public long getDuration(TimeUnit timeUnit) {
                return timeUnit.convert(getDuration(), TimeUnit.NANOSECONDS);
            }

            @Override
            public long getDuration() {
                long end = stopTime == -1 ? System.nanoTime() : stopTime;
                return end - startTime;
            }
        };
    }

    public Timer getDynamicTimer(TagList tagList) {
        MonitorConfig newConfig = config.withAdditionalTags(tagList); // merge in SomeTimerClass instance level tags (assuming that's where "name" comes from)
        Timer unchecked = timerCache.getUnchecked(newConfig);
        return unchecked;
    }

[dmuino]

That's an interesting idea, but I would suggest that you just have a basic MonitorConfig and add some tags to it as needed before calling DynamicTimer.record(config, TimeUnit.SECONDS, duration, timeunit);

For example:

        long start = System.nanoTime();
        MonitorConfig config = MonitorConfig.builder("someName").withTag("k1", "v1").build();
        int i = (new Random()).nextInt(10);
        long now = System.nanoTime() + 42; // some random duration
        if (i < 8) {
            // everything ok
            DynamicTimer.record(config, TimeUnit.SECONDS, now - start, TimeUnit.NANOSECONDS);
        } else {
            // some error
            Tag errTag = new BasicTag("error", "someErr");
            DynamicTimer.record(config.withAdditionalTag(errTag),
                    TimeUnit.SECONDS, now - start, TimeUnit.NANOSECONDS);
        }

In Spectator we created an API fixing some mistakes made in Servo, and with some improvements based on our experience. We got rid of the concept of Stopwatch since it didn't add much value in our opinion:

https://github.com/Netflix/spectator/blob/master/spectator-api/src/main/java/com/netflix/spectator/api/Timer.java