discovery: add rails service name detector #30091
Conversation
Yumasi
force-pushed
the
guillaume.pagnoux/USMON-1233-ruby-service-name-generation
branch
from
0254171
to
3eee457
Compare
Yumasi
added
team/usm
Test changes on VMUse this command from test-infra-definitions to manually test this PR changes on a VM: inv create-vm --pipeline-id=47154650 --os-family=ubuntuNote: This applies to commit bf02b55 |
Regression Detector |
Yumasi
force-pushed
the
guillaume.pagnoux/USMON-1233-ruby-service-name-generation
branch
2 times, most recently
from
c2e1b5f
to
535027d
Compare
Yumasi
force-pushed
the
guillaume.pagnoux/USMON-1233-ruby-service-name-generation
branch
from
535027d
to
eb3ca59
Compare
pkg/collector/corechecks/servicediscovery/usm/testdata/root/testdata/application.rb
Outdated
Show resolved
Hide resolved
pkg/collector/corechecks/servicediscovery/usm/testdata/root/testdata/application_invalid.rb
Outdated
Show resolved
Hide resolved
| reader, err := SizeVerifiedReader(file) | ||
| if err != nil { | ||
| log.Debugf("skipping application.rb (%q): %v", filename, err) | ||
| return "", true |
There was a problem hiding this comment.
we will have a service without name, it does not make sense to me
There was a problem hiding this comment.
While I totally agree with your comment, I do not think it should be the role of the rails detector (or any detectors) to handle that case.
This is also an issue for other detectors (e.g the Gunicorn one) we have and, IMO, should be handled in ExtractServiceMetadata to call a fallback mechanism (simpleDetector?).
There was a problem hiding this comment.
It might be the responsibility of the caller, as long as we guarantee when we zoom out that we won't have services without names, I'm fine with any implementation
| bytes, err := io.ReadAll(reader) | ||
| if err != nil { | ||
| log.Debugf("unable to read application.rb (%q): %v", filename, err) | ||
| return "", true |
There was a problem hiding this comment.
| snake := matchFirstCap.ReplaceAllString(pascalCasedWord, "${1}_${2}") | ||
| snake = matchAllCap.ReplaceAllString(snake, "${1}_${2}") |
There was a problem hiding this comment.
operations on strings are expensive, especially when regex is involved
can't we make a single call to ReplaceAllString?
There was a problem hiding this comment.
I looked a bit into this, and AFAICT, doing it in one call to ReplaceAllString would require lookahead (to find the next word first upper case letter, without actually matching it), which Go's regexp does not support.
So if we want to avoid doing that it looks like we need to make our own Pascal->Snake case converter.
| { | ||
| name: "name is found", | ||
| path: "./testdata/application.rb", | ||
| expected: "rails_hello", | ||
| }, | ||
| { | ||
| name: "name not found", | ||
| path: "./testdata/application_invalid.rb", | ||
| expected: "", | ||
| }, |
There was a problem hiding this comment.
as my other comment suggested, let's work on the test cases to gain full coverage of the function
What does this PR do?
Port the service naming logic for RubyOnRails application. from our injector. It should behave in the same way. This new detector is called when the command name extracted from the command line is
puma(the command line of the application gets replaced when starting a RoR server).Motivation
USMON-1233
Describe how to test/QA your changes
Handled by E2E test.
Possible Drawbacks / Trade-offs
Additional Notes