Skip to content

Conversation

@tylerbenson
Copy link
Contributor

@tylerbenson tylerbenson commented Sep 7, 2017

This PR upgrades our version of OT to the recently released 0.31.0. This is a breaking change for people depending on the OpenTracing API, which is why it's included in the 0.3.0 release and the published module has been renamed.

@palazzem
Copy link
Contributor

palazzem commented Sep 8, 2017

@tylerbenson we should consider that the final solution will be the ScopeManager and Scope PR, but the interface is this one. Both approaches are good to me, and the Scope-whatever naming is definitely better. Nothing that worries me here.

@palazzem
Copy link
Contributor

@tylerbenson I think this one is obsolete right?

@palazzem palazzem requested review from palazzem and removed request for clutchski October 12, 2017 09:57
@palazzem palazzem added the comp: core Tracer core label Oct 12, 2017
@tylerbenson tylerbenson changed the title Changes required for supporting OT API proposal Changes required for supporting OT 0.31.0-RC1 Oct 27, 2017
@tylerbenson
Copy link
Contributor Author

@palazzem I've rebased and made the necessary changes for it to compile against the RC, though as noted, the tests won't pass due to the contrib instrumentation not being updated yet.

@tylerbenson tylerbenson changed the title Changes required for supporting OT 0.31.0-RC1 Changes required for supporting OT 0.31.0-RC2 Jan 2, 2018
@tylerbenson
Copy link
Contributor Author

tylerbenson commented Jan 2, 2018

I have just rebased off master and updated to the RC2. Still have a couple of contribs that aren't updated yet. For now I've ignored those failing tests.

We still need to do some work to split this out into a dd-trace-ot package or something. Hence marked as don't merge.

@tylerbenson tylerbenson added the tag: do not merge Do not merge changes label Jan 2, 2018
@tylerbenson tylerbenson removed the tag: do not merge Do not merge changes label Jan 4, 2018
@tylerbenson tylerbenson requested a review from realark January 4, 2018 11:49
@palazzem palazzem added this to the 0.3.0 milestone Jan 4, 2018
Copy link
Contributor

@realark realark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good.

I don't think we can merge until the opentracing contribs are updated, as those are injected and invoked by the instrumentation at runtime.

@tylerbenson
Copy link
Contributor Author

Oh yeah... forgot about those. I'll follow up with the community to see if someone is working on updating them.

@tylerbenson tylerbenson added the tag: do not merge Do not merge changes label Jan 4, 2018
@tylerbenson tylerbenson force-pushed the tyler/ot-proposal branch 2 times, most recently from 88f619b to d75314a Compare January 12, 2018 14:41
@tylerbenson tylerbenson removed the tag: do not merge Do not merge changes label Jan 12, 2018
@tylerbenson
Copy link
Contributor Author

@realark @palazzem per my request, they published RC versions of those modules with 0.31 support. I've updated to those here. This should be ready for review again.

realark
realark previously approved these changes Jan 16, 2018
Also updates all the contrib versions that we currently depend on to their 0.31.0 supporting versions.
@tylerbenson tylerbenson changed the title Changes required for supporting OT 0.31.0-RC2 Changes required for supporting OT 0.31.0 Jan 16, 2018
@realark realark self-requested a review January 16, 2018 15:17
@tylerbenson tylerbenson merged commit 55c8fdd into master Jan 16, 2018
@tylerbenson tylerbenson deleted the tyler/ot-proposal branch January 16, 2018 16:06
tylerbenson added a commit that referenced this pull request Dec 3, 2020
Currently fails with the following exception:

```
Bad access to protected data in invokevirtual
Exception Details:
  Location:
    io/netty/util/concurrent/GlobalEventExecutor.execute(Ljava/lang/Runnable;)V @76: invokevirtual
  Reason:
    Type 'java/util/concurrent/AbstractExecutorService' (current frame, stack[0]) is not assignable to 'io/netty/util/concurrent/GlobalEventExecutor'
  Current Frame:
    bci: @76
    flags: { }
    locals: { 'io/netty/util/concurrent/GlobalEventExecutor', 'java/lang/Runnable' }
    stack: { 'java/util/concurrent/AbstractExecutorService', 'java/lang/Runnable', null }
  Bytecode:
    0x0000000: 2bc1 0111 9a00 2201 2ba5 001d b201 172b
    0x0000010: b801 1d9a 0013 2bb6 0048 b601 2313 0125
    0x0000020: b601 2b99 0006 a700 3a2b c101 2d99 0012
    0x0000030: bb01 2f59 2bb8 0133 b701 354c a700 212a
    0x0000040: c101 3799 0010 2ac0 0137 2b01 b601 3b4c
    0x0000050: a700 0dbb 013d 592b 01b7 0140 4ca7 0003
    0x0000060: 2a4d 2b4e 2dc7 000d bb00 a859 12a9 b700
    0x0000070: acbf 2c2d b700 d62c b600 d89a 0007 2cb7
    0x0000080: 00db a700 0301 4da7 0004 4d01 2ca5 0015
    0x0000090: 2bc1 0111 9900 0e2b c001 1104 b901 4402
    0x00000a0: 0057 a700 032c c600 052c bfb1
  Exception Handler Table:
    bci [100, 133] => handler: 138
  Stackmap Table:
    same_frame(@38)
    same_frame(@41)
    same_frame(@63)
    same_frame(@83)
    same_frame(@93)
    same_frame(@96)
    append_frame(@100,Object[#2],Object[#126])
    same_frame(@114)
    same_frame(@130)
    full_frame(@133,{Object[#2],Object[#126]},{})
    same_locals_1_stack_item_frame(@138,Object[#271])
    append_frame(@139,Object[#271])
    same_frame(@162)
    same_frame(@165)
    same_frame(@171)
```
jpbempel pushed a commit that referenced this pull request May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp: core Tracer core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants