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.

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

Merged
merged 5 commits into from
Aug 10, 2022

Conversation

ahayworth
Copy link
Contributor

@ahayworth ahayworth commented Jun 5, 2022

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.

@robertlaurin
Copy link
Contributor

For discussion: Do we actually even want this?

🤷 if we're going to run jruby in CI I don't see why not this.

@ahayworth
Copy link
Contributor Author

Fair enough 😄

@plantfansam
Copy link
Contributor

Yeah, I think it makes sense to support pretty much all possible Ruby implementations. +1 for adding this to CI!

Copy link
Contributor

@plantfansam plantfansam left a 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.

shell: bash
run: |
echo "::set-output name=skip::false"
[[ "${{ matrix.gem }}" == "opentelemetry-instrumentation-que" ]] && echo "::set-output name=skip::true"
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed.

@ahayworth
Copy link
Contributor Author

@plantfansam That's a great idea - and perhaps doing it for jruby too is also a good idea... I'm not sure how much effort the different gems would be, though.

@plantfansam
Copy link
Contributor

@plantfansam That's a great idea - and perhaps doing it for jruby too is also a good idea... I'm not sure how much effort the different gems would be, though.

SGTM. Will make tickets for truffleruby and jruby

@plantfansam
Copy link
Contributor

Hi @ahayworth! Sorry for the trouble, but with the opentelemetry-ruby-contrib split, I think we'll need to chop this PR up into contrib-relevant and this-repo-relevant pieces. There will probably be a merge conflict once #1319 makes it into main.

@ahayworth
Copy link
Contributor Author

Hi @ahayworth! Sorry for the trouble, but with the opentelemetry-ruby-contrib split, I think we'll need to chop this PR up into contrib-relevant and this-repo-relevant pieces. There will probably be a merge conflict once #1319 makes it into main.

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.
ahayworth added a commit to ahayworth/opentelemetry-ruby-contrib that referenced this pull request Jun 21, 2022
...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.
ahayworth added a commit to open-telemetry/opentelemetry-ruby-contrib that referenced this pull request Jun 22, 2022
...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 <370182+plantfansam@users.noreply.github.com>
bautrey37 pushed a commit to bautrey37/opentelemetry-ruby-contrib that referenced this pull request Jun 28, 2022
...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 <370182+plantfansam@users.noreply.github.com>
bautrey37 pushed a commit to bautrey37/opentelemetry-ruby-contrib that referenced this pull request Jun 28, 2022
...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 <370182+plantfansam@users.noreply.github.com>
Copy link
Contributor

@plantfansam plantfansam left a comment

Choose a reason for hiding this comment

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

🐑 🇮🇹

@fbogsany fbogsany merged commit d3990c6 into open-telemetry:main Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants