[autoinstrumentation] add ruby autoinstrumentation #3756
Conversation
|
@xuan-cao-swi thank you for the contribution! It's clear you put a lot of effort into it. That said, before we can proceed with code review, we'll need to discuss how exactly we want to go about introducing this new autoinstrumentation to the operator. Could you open an issue and describe how it works? Adding a new instrumentation is a very big change with major implications for the maintainability of this project, and we need to make sure we do it right. |
| @@ -0,0 +1,140 @@ | |||
| # frozen_string_literal: true | |||
There was a problem hiding this comment.
could this code live in the ruby SDK instead?
There was a problem hiding this comment.
Sure, I was looking at nodejs autoinstrumentation and now I just saw the latest pr removed that ts script (will do the same)
There was a problem hiding this comment.
Should we make that change in the ruby auto-inst prior to merging this?
|
Would it make sense for you to step up as codeowner of those files moving forward to help maintain Ruby auto-instrumentation? |
| @@ -0,0 +1,140 @@ | |||
| # frozen_string_literal: true | |||
Co-authored-by: Antoine Toulme <antoine@toulme.name>
Yes, I'd like to help maintain the ruby auto-instrumentation. |
|
I've added this point to the agenda for today's SIG meeting. Anyone interested is encouraged to join in on the discussion. You can find the calendar here. |
main.go
Outdated
| @@ -151,6 +153,7 @@ func main() { | |||
| pflag.BoolVar(&enableDotNetInstrumentation, constants.FlagDotNet, true, "Controls whether the operator supports dotnet auto-instrumentation") | |||
| pflag.BoolVar(&enableGoInstrumentation, constants.FlagGo, false, "Controls whether the operator supports Go auto-instrumentation") | |||
| pflag.BoolVar(&enablePythonInstrumentation, constants.FlagPython, true, "Controls whether the operator supports python auto-instrumentation") | |||
| pflag.BoolVar(&enableRubyInstrumentation, constants.FlagRuby, true, "Controls whether the operator supports ruby auto-instrumentation") | |||
There was a problem hiding this comment.
set to false initially and we can turn it on by default later
|
Please, add the needed documentation to |
…entelemetry-operator into ruby-autoinstrumentation
There was a problem hiding this comment.
This all looks good to me, I think there's just two things missing:
- I think it would be great to add a README into the
autoinstrumentation/rubythat describes the agreement the Ruby SIG maintainers have in keeping it up to date, maintaining this etc. - We have a pattern for test e2e-instrumentation apps, and I think it would be great to add one in for ruby, these are usually toy examples that don't do much other than verify that auto-instrumentation is working for the language. I think it would be okay to do this in a followup, but we should make an issue for that either way.
| @@ -232,6 +236,30 @@ type Python struct { | |||
| Resources corev1.ResourceRequirements `json:"resourceRequirements,omitempty"` | |||
| } | |||
|
|
|||
| type Ruby struct { | |||
There was a problem hiding this comment.
Note: I think for the future we should probably just make a common struct called "CommonLanguageDetails" or something like that rather than needing to copy-paste this thing everytime. (not asking for that to be done with this, going to convert this comment into an issue)
| @@ -0,0 +1,140 @@ | |||
| # frozen_string_literal: true | |||
There was a problem hiding this comment.
Should we make that change in the ruby auto-inst prior to merging this?
Thanks @jaronoff97 , I have the e2e instrumentation test case ready. Once this one get accepted/merged, I will create a follow up PR.
I am still working with otel ruby maintainer to push the script as a new ruby gem. |
Description:
Add the initial version of autoinstrumentation ruby (image and crd change).
Link to tracking Issue(s):
Testing:
e2e-instrumentationande2e-multi-instrumentationare performed; but hopefully they can be added after autoinstrumentation-ruby image gets publish first.Documentation:
Added doc in docs/api/instrumentations.md