[receiver/oracledb] Add service.instance.id resource attribute to metrics and logs#42421
Conversation
…d query_sample collection
Oracledbreceiver events
Oracle additional attributes
| if err == nil && (strings.EqualFold(host, "localhost") || net.ParseIP(host).IsLoopback()) { | ||
| host, err = os.Hostname() | ||
| } | ||
|
|
||
| if err != nil { | ||
| logger.Warn("Failed to compute service.instance.id", zap.Error(err)) | ||
| if port == "" { | ||
| return fallback | ||
| } | ||
| return "unknown:" + port | ||
| } |
There was a problem hiding this comment.
I would prefer handle the err != nil first.
Something like
host, port, err := net.SplitHostPort(hostString)
if err != nil {}
| # These lines will be padded with 2 spaces and then inserted directly into the document. | ||
| # Use pipe (|) for multiline entries. | ||
| subtext: | | ||
| The `service.instance.id` attribute is added in the format `<host>:<port>/<service>` to uniquely identify |
There was a problem hiding this comment.
IIUC, the service.instance.id should not include the service. And this service has already been pushed as oracledb.instance.name. Could you elaborate on the reason for including service in the service.instance.id?
There was a problem hiding this comment.
A sample Oracle connection string would be oracle://system:password123@127.0.0.1:1521/XEPDB1
In the above string the XEPDB1 is a PDB (Pluggable database) instance.
In Oracle Enterprise Edition users can spin up upto ~250 PDB instances, and they all will live under the same host and port.
This makes it necessary to have the PDB name be attached to the service.instace.id that we create.
| if strings.EqualFold(host, "localhost") || net.ParseIP(host).IsLoopback() { | ||
| host, err = os.Hostname() | ||
| } | ||
| if err != nil { |
There was a problem hiding this comment.
I think this if should be next to the statement that potentially sets err. It's harder to follow when they're separated.
There was a problem hiding this comment.
If I'm following the logic properly, service.instance.id will never be empty. Can we update test data to match functionality? (Same comment for expectedSamplesFile.yaml)
There was a problem hiding this comment.
I'm still seeing empty values here, perhaps a commit wasn't pushed?
There was a problem hiding this comment.
Think so. Updated now!
crobert-1
left a comment
crobert-1
left a comment
There was a problem hiding this comment.
Looks good, just a few more nits and requests 👍
| return constructInstanceID(host, port, service) | ||
| } | ||
|
|
||
| func constructInstanceID(host, port, service string) (id string) { |
There was a problem hiding this comment.
This method assumes valid host and port, should it have some validation involved? As I pointed out there was a case where host may be empty, it'd probably be good if we set defaults if empty or invalid strings were passed in, then construct the instance ID.
There was a problem hiding this comment.
Modified the method to set defaults for host and port.
There was a problem hiding this comment.
I'm still seeing empty values here, perhaps a commit wasn't pushed?
Co-authored-by: Curtis Robert <crobert@splunk.com>
Co-authored-by: Curtis Robert <crobert@splunk.com>
Co-authored-by: Curtis Robert <crobert@splunk.com>
Co-authored-by: Curtis Robert <crobert@splunk.com>
|
@crobert-1 Comments addressed, please check. thanks |
|
Thank you for your contribution @sv-splunk! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. |
4 participants
Description
The
service.instance.idattribute is added into resourceAttributes of metrics and logs to uniquely identifyOracle db hosts. The format of id is
<host>:<port>/<service>Link to tracking issue
Fixes #42402
Testing
Unit tests are added/updated
Documentation