Skip to content

Tracing-aware functions, bifunctions, suppliers, consumers, biconsumers#15

Open
mattsills wants to merge 1 commit intopalantir:developfrom
mattsills:ms/more-tracing-aware-stuff
Open

Tracing-aware functions, bifunctions, suppliers, consumers, biconsumers#15
mattsills wants to merge 1 commit intopalantir:developfrom
mattsills:ms/more-tracing-aware-stuff

Conversation

@mattsills
Copy link

@mattsills mattsills commented Oct 1, 2018

No description provided.

@mattsills mattsills requested a review from a team as a code owner October 1, 2018 14:00
@iamdanfox
Copy link
Contributor

iamdanfox commented Oct 1, 2018

Out of curiosity, can you paste a code snippet that you're intending to use these new Tracers.wrap methods?

@mattsills
Copy link
Author

mattsills commented Oct 26, 2018 •
edited
Loading

@iamdanfox anything involving chaining futures.

CompletableFuture.supplyAsync(..., executor).thenApply(res -> ...);

CompletableFuture.supplyAsync(..., executor).thenApply(trace(res -> ...));

@pkoenig10
Copy link
Member

pkoenig10 commented Dec 20, 2018

This won't actually work if you try to create a Supplier.

For example, this code:

Callable<Integer> callable = Tracers.wrap(() -> 42);
Supplier<Integer> supplier = Tracers.wrap(() -> 42);

will fail with the following errors:

reference to wrap is ambiguous

incompatible types: no instance(s) of type variable(s) V exist so that Callable conforms to Supplier

@dansanduleac
Copy link
Contributor

dansanduleac commented Nov 12, 2019

@mattsills do you still need this?
Also, @pkoenig10 raised a point that this breaks, can you comment on whether that's the case or not?

@CRogers CRogers removed the request for review from a team December 11, 2019 11:20
carterkozak pushed a commit that referenced this pull request Mar 9, 2022
Simplify Tracer.shouldObserve to avoid JIT friction
a708713 
Uses a simple condition rather than a switch statement

After:
`javap -c -p ./tracing/build/classes/java/main/com/palantir/tracing/Tracer.class`
```
  private static boolean shouldObserve(com.palantir.tracing.Observability);
    Code:
       0: aload_0
       1: getstatic     #10                 // Field com/palantir/tracing/Observability.SAMPLE:Lcom/palantir/tracing/Observability;
       4: if_acmpeq     25
       7: aload_0
       8: getstatic     #11                 // Field com/palantir/tracing/Observability.UNDECIDED:Lcom/palantir/tracing/Observability;
      11: if_acmpne     29
      14: getstatic     #12                 // Field sampler:Lcom/palantir/tracing/TraceSampler;
      17: invokeinterface #13,  1           // InterfaceMethod com/palantir/tracing/TraceSampler.sample:()Z
      22: ifeq          29
      25: iconst_1
      26: goto          30
      29: iconst_0
      30: ireturn
```

Before:
`javap -c -p ./tracing/build/classes/java/main/com/palantir/tracing/Tracer.class`
```
  private static boolean shouldObserve(com.palantir.tracing.Observability);
    Code:
       0: getstatic     #10                 // Field com/palantir/tracing/Tracer$1.$SwitchMap$com$palantir$tracing$Observability:[I
       3: aload_0
       4: invokevirtual #11                 // Method com/palantir/tracing/Observability.ordinal:()I
       7: iaload
       8: tableswitch   { // 1 to 3
                     1: 36
                     2: 38
                     3: 40
               default: 49
          }
      36: iconst_1
      37: ireturn
      38: iconst_0
      39: ireturn
      40: getstatic     #12                 // Field sampler:Lcom/palantir/tracing/TraceSampler;
      43: invokeinterface #13,  1           // InterfaceMethod com/palantir/tracing/TraceSampler.sample:()Z
      48: ireturn
      49: new           #14                 // class com/palantir/logsafe/exceptions/SafeIllegalArgumentException
      52: dup
      53: ldc           #15                 // String Unknown observability
      55: iconst_1
      56: anewarray     #16                 // class com/palantir/logsafe/Arg
      59: dup
      60: iconst_0
      61: ldc           #17                 // String observability
      63: aload_0
      64: invokestatic  #18                 // Method com/palantir/logsafe/SafeArg.of:(Ljava/lang/String;Ljava/lang/Object;)Lcom/palantir/logsafe/SafeArg;
      67: aastore
      68: invokespecial #19                 // Method com/palantir/logsafe/exceptions/SafeIllegalArgumentException."<init>":(Ljava/lang/String;[Lcom/palantir/logsafe/Arg;)V
      71: athrow
```
carterkozak pushed a commit that referenced this pull request Mar 9, 2022
Simplify Tracer.shouldObserve to avoid JIT friction
67fe357 
Uses a simple condition rather than a switch statement

After:
`javap -c -p ./tracing/build/classes/java/main/com/palantir/tracing/Tracer.class`
```
  private static boolean shouldObserve(com.palantir.tracing.Observability);
    Code:
       0: aload_0
       1: getstatic     #10                 // Field com/palantir/tracing/Observability.SAMPLE:Lcom/palantir/tracing/Observability;
       4: if_acmpeq     25
       7: aload_0
       8: getstatic     #11                 // Field com/palantir/tracing/Observability.UNDECIDED:Lcom/palantir/tracing/Observability;
      11: if_acmpne     29
      14: getstatic     #12                 // Field sampler:Lcom/palantir/tracing/TraceSampler;
      17: invokeinterface #13,  1           // InterfaceMethod com/palantir/tracing/TraceSampler.sample:()Z
      22: ifeq          29
      25: iconst_1
      26: goto          30
      29: iconst_0
      30: ireturn
```

Before:
`javap -c -p ./tracing/build/classes/java/main/com/palantir/tracing/Tracer.class`
```
  private static boolean shouldObserve(com.palantir.tracing.Observability);
    Code:
       0: getstatic     #10                 // Field com/palantir/tracing/Tracer$1.$SwitchMap$com$palantir$tracing$Observability:[I
       3: aload_0
       4: invokevirtual #11                 // Method com/palantir/tracing/Observability.ordinal:()I
       7: iaload
       8: tableswitch   { // 1 to 3
                     1: 36
                     2: 38
                     3: 40
               default: 49
          }
      36: iconst_1
      37: ireturn
      38: iconst_0
      39: ireturn
      40: getstatic     #12                 // Field sampler:Lcom/palantir/tracing/TraceSampler;
      43: invokeinterface #13,  1           // InterfaceMethod com/palantir/tracing/TraceSampler.sample:()Z
      48: ireturn
      49: new           #14                 // class com/palantir/logsafe/exceptions/SafeIllegalArgumentException
      52: dup
      53: ldc           #15                 // String Unknown observability
      55: iconst_1
      56: anewarray     #16                 // class com/palantir/logsafe/Arg
      59: dup
      60: iconst_0
      61: ldc           #17                 // String observability
      63: aload_0
      64: invokestatic  #18                 // Method com/palantir/logsafe/SafeArg.of:(Ljava/lang/String;Ljava/lang/Object;)Lcom/palantir/logsafe/SafeArg;
      67: aastore
      68: invokespecial #19                 // Method com/palantir/logsafe/exceptions/SafeIllegalArgumentException."<init>":(Ljava/lang/String;[Lcom/palantir/logsafe/Arg;)V
      71: athrow
```

Benchmarks show improvement within variance:

Benchmarks AFTER:
```
Benchmark                             (observability)  Mode  Cnt    Score     Error  Units
TracingBenchmark.traceWithSingleSpan           SAMPLE  avgt    3  533.719 ± 164.796  ns/op
TracingBenchmark.traceWithSingleSpan    DO_NOT_SAMPLE  avgt    3  100.138 ±  35.819  ns/op
TracingBenchmark.traceWithSingleSpan        UNDECIDED  avgt    3  122.807 ±  64.274  ns/op
```

Benchmarks BEFORE:
```
Benchmark                             (observability)  Mode  Cnt    Score    Error  Units
TracingBenchmark.traceWithSingleSpan           SAMPLE  avgt    3  522.865 ± 65.310  ns/op
TracingBenchmark.traceWithSingleSpan    DO_NOT_SAMPLE  avgt    3  112.647 ± 48.826  ns/op
TracingBenchmark.traceWithSingleSpan        UNDECIDED  avgt    3  118.643 ±  6.733  ns/op
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mattsills @iamdanfox @pkoenig10 @dansanduleac

Comments