refactor infinite tracing for improved clarity and simplicity, add su… #180
Conversation
…pport for flaky code test configuration
…pport for flaky code test configuration
| } | ||
|
|
||
| @VisibleForTesting | ||
| static V1.Span convert(SpanEvent spanEvent) { |
There was a problem hiding this comment.
I would prefer the span conversion process to remain in a separate class. I know we discussed it is a straight forward and simple conversion. I think it is separate enough of a responsibility to organize on its own.
There was a problem hiding this comment.
Sure I can do that, but before I do, let me just explain my reasoning for why I didn't do it initially.
IMO, decomposition is about breaking large problems into smaller problems of singular concern, and a separate java class files isn't always the best way to accomplish that. They can reduce readability (i.e. having to look at 5 files to figure out what's happening is harder to undertstand than 5 functions in a single file). And they can encourage consumption outside of code outside its intended use (i.e. a private function in a class file sends a clear signal to maintainers that only the owning class should consume it while a separate class might more readily reused). They can help reduce clutter, but at 138 lines I don't think that this class is suffering from that.
Anyway, that's my point of view in general, but in this case I don't think breaking it out is such a bad thing.
There was a problem hiding this comment.
Appreciate the insight and this will help me in the future! At times, I've gone through what seems a bit much of code indirection and agree with your point it is hard to read.
…lic-java-agent into refactor-infinite-tracing
There was a problem hiding this comment.
I might be a bit too close to these changes as @jack-berg and have discussed them at length. I think these changes do improve the code quality, readability, and reduces complexity.
If you plan to manual test for proper behavior, here are some notes that might be helpful.
-
Use the
flaky_codeagent config setting for testing. This header metadata is only observed by the staging Trace Observer, so it will not work with a production Trace Observer. -
Testing
FAILED_PRECONDITIONwill be just verifying we disconnect from the Trace Observer and reconnect to the same one. If you want to verify we do reconnect with a different IP address, the ∞T team has offered to help with that test because it is easy for them to do.
There was a problem hiding this comment.
I didn't have much exposure to this part of the codebase before the refactoring so I can't say that I have much of a feel for why it was refactored. That said, the refactored code looks good to me. Overall it looks to have reduced the number of classes a good amount without creating "god" classes, which is nice.
Has this been manually tested and verified to work (old and new functionality alike)? And have the AITs been run against this branch? If so, then let's go ahead and merge this.
| status = statusRuntimeException.getStatus(); | ||
| metricAggregator.incrementCounter("Supportability/InfiniteTracing/Span/gRPC/" + status.getCode()); | ||
| if (status.getCode() == Status.Code.UNIMPLEMENTED) { | ||
| // See: https://source.datanerd.us/agents/agent-specs/blob/master/Infinite-Tracing.md#unimplemented |
There was a problem hiding this comment.
ugg I wish these specs were public
There was a problem hiding this comment.
Has this been manually tested and verified to work (old and new functionality alike)? And have the AITs been run against this branch?
Yes - lots of manual testing, and the AITs have passed.
| @@ -19,6 +19,7 @@ | |||
| public static final String TRACE_OBSERVER = "trace_observer"; | |||
| public static final String SPAN_EVENTS = "span_events"; | |||
| public static final String FLAKY_PERCENTAGE = "_flakyPercentage"; | |||
| public static final String FLAKY_CODE = "_flakyCode"; | |||
There was a problem hiding this comment.
Does the _flakyCode config need to be publicly documented for customer use?
There was a problem hiding this comment.
Good question! No, the headers included by _flakyCode and _flakyPercentage only impact interactions with the staging environment.
Refactor infinite tracing for improved clarity and simplified. Refactor implies that the code has been reorganized but is functionally equivalent. This is mostly true for this PR. The changes in behavior include:
FAILED_PRECONDITIONSstatus is returned._flakyCodeconfig, which allows for debugging error conditions with_flakyPercentage.Closes #164.
Closes #160.