Start implementation of new tracer API

[mar-kolya]

There are still quite a few TODOs and work left in general, but this seems to be a reasonable 'stopping point' to start reviewing these changes.

[mar-kolya]

@realark this should be ready for review now

[tylerbenson]

Lots of comments...

[tylerbenson]

Originally when we called it just Interceptor, it was inside the class it was intercepting. With it being stand-alone, it doesn't have that context in the name.

[mar-kolya]

We also now have single Interceptor class instead of multiple, so I kind of feel it makes sense to pull it out.

[tylerbenson]

I'd like to group things better into packages. This could probably be grouped with Clock into a single package.

[mar-kolya]

I was actually considering that along with some other 'repackaging' - i.e. I was thinking making an internal package to 'hide' all implementational stuff. And yeah, time related stuff can go into its own package. The only thing that prevents doing that currently is the fact that error reporting stuff is 'package private' and it would be invisible if we move that - so I was hoping to postpone this repackaging until we have logging/error reporting better ironed out. Objections?

[tylerbenson]

That's fine. I hate that java doesn't allow sub-packages to access parent package private stuff.

[tylerbenson]

Should we consider a builder pattern for Tracer instead of providing all these constructors?

[mar-kolya]

We probably should have builder. There is more work needed on the tracer since we also need some reasonable way to gather up and provide all interceptors to it. Maybe this can be hashed out at later stages?

[tylerbenson]

Why does clock need to maintain a reference to the source tracer? Is this only so Timestamp can reportError?

[mar-kolya]

Yes, right now that is the only reason.

[tylerbenson]

I think we might need to have some sort of rate limiting on these kind of log messages as a misbehaving integration could easily cause a machine to fill up it's disk.

[mar-kolya]

I was planning to do a second iteration over this whole error/warning reporting part, so yeah having ratelimiting is a good idea, but maybe it can be left for another PR?

[tylerbenson]

This is interesting... what does this do?

[mar-kolya]

The general idea in EqualsVerifier is to make sure that class adheres to 'standard' equals/hashCode contract. We have to disable some stuff - in this case we do not declare Clock final so we disable checks related to potential Clocks child classes.

[tylerbenson]

Cool. Didn't know you could add comments to the when/then blocks.

[realark]

(Still reading through this. Thoughts so far)

[realark]

👍

This is great. agent-bootstrap will be a great way to share internal tools to instrumentation and the core tracer. I added the project to list of potential renames (since it's more of a general util at this point and not agent-specific.

[realark]

I'm okay with using finalizers, but given this caveat we should wrap them try - catch(Throwable) to ensure we don't miss errors.

[realark]

With our new concept of invalid traces, can we remove the trace count incrementer and leave that up to the writer?

[mar-kolya]

Yeah, I've tried that. And then @tylerbenson had pointed out that we still need to count traces that have been discarded by sampler.

[realark]

Looks good! Left a few small suggestions.