Fixes #688: Change service.instance.id serialization to string #793
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #793 +/- ##
==========================================
- Coverage 73.20% 73.15% -0.06%
==========================================
Files 64 64
Lines 1941 1941
==========================================
- Hits 1421 1420 -1
- Misses 520 521 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
|
Sorry about the lack of docs on testing. You mention not being able to setup rebar3, can you give some more details? Running tests should just require running |
|
Actually we have service id all wrong. I don't know when this changed but the spec for it is different from when we implemented this and so our implementation needs to change to match https://github.com/open-telemetry/semantic-conventions/blob/d9d1e21a5bbb37fe8facea323491d59342a5f810/docs/attributes-registry/service.md#service-instance-id Would you like to continue the work? Otherwise I can hop on this right away as its a bug and major difference to the spec. Thanks for bringing it to our attention! |
|
@tsloughter I am not highly experienced in Erlang and am focusing on beginner-friendly issues for my contributions. Please feel free to proceed with the work. I would greatly appreciate any assistance in ensuring this PR is accurate and ready for merging. |
|
@tsloughter I have set up Rebar3; a system restart was required to update the environment variables. I’ve implemented the fixes in the latest commit, but I'm encountering errors when running |
|
@Annosha hey, sorry, that link to uploaded error log doesn't work. Could you just push the changes so we can see if they pass in github CI? |
|
@tsloughter sorry about the file, here is the error log. And I have pushed changes here in this PR tho. |
|
@Annosha oh, sorry for not getting back to you! To run the full test suite you need to have a collector running. You can do that with docker compose in the project. If you run that do things pass? I haven't gotten around to fixing up Sorry again for dropping the ball, and thanks! |
Fixes #688
This PR aims to update Serialization Logic by:
Converting
service.instance.idto a string before it is sent in protobuf messages. Any occurrence ofservice.instance.idin \otel_resource_detector.erl being assigned an integer value was modified to convert it to a binary string format, ensuring compliance with the expected data type.Note: As a new contributor, I encountered difficulties in running tests due to a lack of documentation regarding the testing tools in the OpenTelemetry Erlang repository. Consequently, I am relying on CI/CD tests for my initial pull request. I would greatly appreciate any guidance on setting up the testing environment.
For your reference, I am using Windows with the following Erlang version: Erlang (SMP, ASYNC_THREADS) (BEAM) emulator version 15.1.2. I am currently facing challenges in setting up Rebar3.