Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

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

wants to merge 1 commit into from

Conversation

mattsills
Copy link

No description provided.

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

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

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

@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 added a commit that referenced this pull request Mar 9, 2022
@carterkozak
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 added a commit that referenced this pull request Mar 9, 2022
@carterkozak
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