-
Notifications
You must be signed in to change notification settings - Fork 252
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.
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
feat: support truffleruby #1295
feat: support truffleruby #1295
Conversation
c01403f
to
df55bab
Compare
🤷 if we're going to run jruby in CI I don't see why not this. |
Fair enough 😄 |
Yeah, I think it makes sense to support pretty much all possible Ruby implementations. +1 for adding this to CI! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! If the gems enumerated in truffleruby filter
don't pass when run in CI, what do you think about making GH issues for each of them? These could make great tasks for newcomers to the project. I am happy to make those tickets if you think it's a good idea.
.github/workflows/ci.yml
Outdated
shell: bash | ||
run: | | ||
echo "::set-output name=skip::false" | ||
[[ "${{ matrix.gem }}" == "opentelemetry-instrumentation-que" ]] && echo "::set-output name=skip::true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So these gems fail against truffleruby, is that right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed.
@plantfansam That's a great idea - and perhaps doing it for |
SGTM. Will make tickets for truffleruby and jruby |
Hi @ahayworth! Sorry for the trouble, but with the |
It's no trouble at all; I'll update this. |
Does what it says on the tin. By "support" I mean "turn it on in CI and see if everything works." The compatibility list is actually a bit better than JRuby! Notably, I needed to exclude the OTLP exporter to make tests pass. I'm not certain it's a real bug; I just haven't tried to fix those test failures yet.
df55bab
to
448e991
Compare
...and also add back tests for propagators 😱 But this is essentially the instrumentation component of open-telemetry/opentelemetry-ruby#1295 but ported to this repo.
...and also add back tests for propagators 😱 But this is essentially the instrumentation component of open-telemetry/opentelemetry-ruby#1295 but ported to this repo. Co-authored-by: Sam <[email protected]>
...and also add back tests for propagators 😱 But this is essentially the instrumentation component of open-telemetry/opentelemetry-ruby#1295 but ported to this repo. Co-authored-by: Sam <[email protected]>
...and also add back tests for propagators 😱 But this is essentially the instrumentation component of open-telemetry/opentelemetry-ruby#1295 but ported to this repo. Co-authored-by: Sam <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐑 🇮🇹
Does what it says on the tin. By "support" I mean "turn it on in CI and
see if everything works."
The compatibility list is actually a bit better than JRuby!
Notably, I needed to exclude the OTLP exporter to make tests pass. I'm
not certain it's a real bug; I just haven't tried to fix those test
failures yet.
For discussion: Do we actually even want this? I went down a rabbithole today and landed on truffleruby, and thought it might be neat to support it. It seems like it's broadly more compatible than JRuby to me. Digging through past issues and PRs, it looks like we've at least attempted to support this in the past, and then maybe accidentally dropped it over time.