Closes #588 - Added span error status #614
Conversation
There was a problem hiding this comment.
I didn't had a detailed look on the tests but they will be fine I guess :)
Reviewed 6 of 8 files at r1.
Reviewable status: 6 of 8 files reviewed, 8 unresolved discussions (waiting on @JonasKunz)
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/hook/MethodHookGenerator.java, line 151 at r1 (raw file):
isBlank
You can also use isNotBlankinstead of the negation.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/hook/MethodHookGenerator.java, line 153 at r1 (raw file):
isBlank
Here as well: isNotBlank
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/hook/actions/span/SetSpanStatusAction.java, line 17 at r1 (raw file):
*/ @AllArgsConstructor public class SetSpanStatusAction implements IHookAction {
Can you add a unit test for some small tests, e.g. if a span has been entered or not
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/hook/actions/span/SetSpanStatusAction.java, line 28 at r1 (raw file):
enteredSpan
Maybe a bit out of scope here, but can we name this method hasEnteredSpan. I was confused why we are entering a span here, because I dind't read that it was entered.
Imo it is always nice when boolean method are starting with is or has. What do you think?
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/hook/actions/span/SetSpanStatusAction.java, line 30 at r1 (raw file):
if (ctx.enteredSpan()) { Object statusValue = errorStatus.get(context); if (statusValue != null && !Boolean.FALSE.equals(statusValue)) {
You could remove the null check.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/hook/actions/span/SetSpanStatusAction.java, line 43 at r1 (raw file):
@Override public String getName() { return "Set Span Status";
We have different "styles" of naming the existing span actions, e.g.:
- StoreSpanAction - >
Span storing - SetSpanStatusAction ->
Span Attribute Writing - SetSpanStatusAction ->
Set Span Status
Can you unify them and chose a common style?
inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/instrumentation/config/MethodHookConfigurationResolverTest.java, line 164 at r1 (raw file):
.hasSize(2) .containsEntry("attr", "dataX") .containsEntry("attr2", "dataY");
Shouldn't this also contain an error tag?
inspectit-ocelot-documentation/docs/instrumentation/rules.md, line 490 at r1 (raw file):
> The name must exist at the end of the entry section and cannot be set in the exit section. #### Trace Sampling
Nice! Additional headlines makes it much easier to read!
inspectit-ocelot-documentation/docs/instrumentation/rules.md, line 541 at r1 (raw file):
:
, of a rule's tracing section.
inspectit-ocelot-documentation/docs/instrumentation/rules.md, line 553 at r1 (raw file):
of `error-status` can
of the error-status property can...
There was a problem hiding this comment.
Reviewable status: 3 of 14 files reviewed, 7 unresolved discussions (waiting on @mariusoe)
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/hook/MethodHookGenerator.java, line 151 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
isBlankYou can also use
isNotBlankinstead of the negation.
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/hook/MethodHookGenerator.java, line 153 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
isBlankHere as well:
isNotBlank
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/hook/actions/span/SetSpanStatusAction.java, line 17 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Can you add a unit test for some small tests, e.g. if a span has been entered or not
Done.
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/hook/actions/span/SetSpanStatusAction.java, line 28 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
enteredSpanMaybe a bit out of scope here, but can we name this method
hasEnteredSpan. I was confused why we are entering a span here, because I dind't read that it wasentered.
Imo it is always nice when boolean method are starting withisorhas. What do you think?
Agreed
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/hook/actions/span/SetSpanStatusAction.java, line 30 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
You could remove the null check.
I think you missed the negation in the equals check there.
If I removed the null check, a value of null would result in the span being marked as error
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/hook/actions/span/SetSpanStatusAction.java, line 43 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
We have different "styles" of naming the existing span actions, e.g.:
- StoreSpanAction - >
Span storing- SetSpanStatusAction ->
Span Attribute Writing- SetSpanStatusAction ->
Set Span StatusCan you unify them and chose a common style?
Done.
inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/instrumentation/config/MethodHookConfigurationResolverTest.java, line 164 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Shouldn't this also contain an
errortag?
This is the test for the configuration resolver, not for the span action.
E.g. we verify the hook build plan, not the attributes geenrated at runtime.
inspectit-ocelot-documentation/docs/instrumentation/rules.md, line 553 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
of `error-status` canof the
error-statusproperty can...
Done.
There was a problem hiding this comment.
Reviewed 2 of 8 files at r1, 9 of 9 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @JonasKunz and @mariusoe)
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/hook/actions/span/SetSpanStatusAction.java, line 30 at r1 (raw file):
Previously, JonasKunz (Jonas Kunz) wrote…
I think you missed the negation in the
equalscheck there.
If I removed thenullcheck, a value ofnullwould result in the span being marked as error
Yeah, I think I saw that later and I thought I removed this comment again :)
inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/instrumentation/hook/actions/span/SetSpanStatusActionTest.java, line 67 at r2 (raw file):
void throwableStatus() { SetSpanStatusAction action = new SetSpanStatusAction((ctx) -> new Throwable()); doReturn(true).when(ctx).hasEnteredSpan();
Can you add the same test, with hasEntered false? Then there should be zero interaction with the span, right?
There was a problem hiding this comment.
Reviewable status: 12 of 14 files reviewed, 1 unresolved discussion (waiting on @mariusoe)
inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/instrumentation/config/MethodHookConfigurationResolverTest.java, line 164 at r1 (raw file):
Previously, JonasKunz (Jonas Kunz) wrote…
This is the test for the configuration resolver, not for the span action.
E.g. we verify the hook build plan, not the attributes geenrated at runtime.
Done.
There was a problem hiding this comment.
Dismissed @mariusoe from a discussion.
Reviewable status: 12 of 14 files reviewed, all discussions resolved (waiting on @mariusoe)
There was a problem hiding this comment.
Reviewed 1 of 8 files at r1.
Reviewable status: 12 of 14 files reviewed, all discussions resolved (waiting on @mariusoe)
There was a problem hiding this comment.
Reviewed 1 of 2 files at r3.
Reviewable status: 13 of 14 files reviewed, all discussions resolved (waiting on @mariusoe)
There was a problem hiding this comment.
Reviewed 1 of 2 files at r3.
Reviewable status:complete! all files reviewed, all discussions resolved
This change is