fix(exporter-logs-otlp-http): set useHex to true

[Nico385412]

Which problem is this PR solving?

When we use the OTLPLogExporter to send logs to a collector I face this issue:

readLog.traceId: parse trace_id:invalid length for ID, error found

When I look at what the exporter send, traceId look like this x+wKeY+LpUblmgFbiqXBwQ== which is base64. that a bug we should have an hex.

When I look at OTLPTraceExporter i notice it adds an extra parameter true to this function createExportLogsServiceRequest which disable the mapping to base64.

This PR replicate this behavior to OTLPLogExporter

Short description of the changes

createExportLogsServiceRequest function used in OTLPLogExporter now have an extra parameter true to use hex format

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Has been tested in TDD and i had a single new test

• convert -> should call createExportLogsServiceRequest with useHex parameter to true

Checklist:

• Followed the style guidelines of this project
• Unit tests have been added
• Documentation has been updated

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: Nico385412 / name: Nicolas Hansse (e5b0967, c55af60, da79112, 1b5d069, 8b85f87, 6600955, 6209f9a)

[llc1123]

I think we should complete the tests. The current tests are too simple.

In the case of this PR is solving, we should compare the results rather than checking the internal parameters.

Please refer tests in other packages.

[Nico385412]

I think we should complete the tests. The current tests are too simple.

In the case of this PR is solving, we should compare the results rather than checking the internal parameters.

Please refer tests in other packages.

Hi !

I just have a look at other packages and i think no one test the "convert" method.
Since it's just a passthrough i don't know what kind of relevant tests i could implements 😅

Do you have some advice or asserts i should write ?

thank you :D

[llc1123]

Do you have some advice or asserts i should write ?

You export some log records and expect the output to be in correct otlp-http format.

Please refer to tests of exporter-logs-otlp-grpc and exporter-trace-otlp-http.

I just have a look at other packages and i think no one test the "convert" method.

You don't need to test the convert method alone. The function is already tested in otlp-transformer.

[Nico385412]

Thank you for you for your answer.

I'm really sorry but i'm a bit confused by your comment.

You export some log records and expect the output to be in correct otlp-http format.

You don't need to test the convert method alone. The function is already tested in otlp-transformer

Since i don't need to test convert method, and it's already tested in otep-transformer, how can i export log records and expect the result to be in correct otlp-http format ?

As I understand, you ask me to test twice the createExportLogsServiceRequest behavior 😅

[llc1123]

Since i don't need to test convert method, and it's already tested in otep-transformer, how can i export log records and expect the result to be in correct otlp-http format ?

I mean you should:

1. create a http server or sth to receive the output
2. create a OTLPLogExporter
3. call the export method with a prepared test sample and target to the mock server
4. get the result from the mock server
5. assert the result to be expected

[Nico385412]

Since i don't need to test convert method, and it's already tested in otep-transformer, how can i export log records and expect the result to be in correct otlp-http format ?

I mean you should:

1. create a http server or sth to receive the output
2. create a OTLPLogExporter
3. call the export method with a prepared test sample and target to the mock server
4. get the result from the mock server
5. assert the result to be expected

Hi !

I tried to perform the same tests as exporter-logs-otlp-proto does but for no reasons, sinon refuse to stub http module

I tried :

• remove my test and copy paste OTLPLogsExporter.test.ts from exporter-logs-otlp-proto
• keep only the describe(export) tests
• adapt the code

So far my new test is exactly the same as the one from exporter-logs-otlp-proto i don't understand why sinon is able to stub http in one module and not another.

To solve this issue i tried :

• upgrade sinon to 15.0.0 (exporter-logs-otlp-proto has version 15 and exporter-logs-otlp-http 14)
• debug step-by-step

But i dont know what i miss here :/

[llc1123]

https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/exporter-trace-otlp-http/test/node/CollectorTraceExporter.test.ts#L197-L211

[Nico385412]

Ok should be good now, in the end the issue was with esModuleInterop option ...

[llc1123]

LGTM

[codecov]

Codecov Report

Merging #3875 (4c7211a) into main (74393ac) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 4c7211a differs from pull request most recent head 6209f9a. Consider uploading reports for the commit 6209f9a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3875      +/-   ##
==========================================
+ Coverage   92.89%   92.90%   +0.01%     
==========================================
  Files         297      297              
  Lines        8838     8838              
  Branches     1815     1815              
==========================================
+ Hits         8210     8211       +1     
+ Misses        628      627       -1     

Impacted Files	Coverage Δ
...ogs-otlp-http/src/platform/node/OTLPLogExporter.ts	100.00% <100.00%> (+16.66%)	⬆️

... and 1 file with indirect coverage changes

[pichlermarc]

Hi, thanks for fixing this 🙂 Would you mind setting useHex to true in the browser exporter as well (src/platform/browser/OTLPLogExporter.ts)? This way we'll have this bug fixed for good 🙂

[Nico385412]

Hi, thanks for fixing this 🙂 Would you mind setting useHex to true in the browser exporter as well (src/platform/browser/OTLPLogExporter.ts)? This way we'll have this bug fixed for good 🙂

Done :D

[pichlermarc]

thanks 🙂

[pichlermarc]

whoops, looks like a test is still failing 👀

[Nico385412]

Can you try again ? looks weird everything is fine on my laptop :/

[pichlermarc]

Can you try again ? looks weird everything is fine on my laptop :/

The browser tests seem to fail, you can run those with NODE_OPTIONS=--openssl-legacy-provider npm run test:browser.
The reason is that the new tests are using node types that are not available in the browser, so the tests need to be adapted to resemble the tests in https://github.com/open-telemetry/opentelemetry-js/blob/abfe059d72d2a50d993ee7fb5ea50a1c29851a31/experimental/packages/exporter-trace-otlp-http/test/browser/CollectorTraceExporter.test.ts

[erikclairiot]

Facing same issue in our project, any news on this topic ? Thanks @Nico385412

[Nico385412]

@pichlermarc should be good :D

[pichlermarc]

Looks great, thanks for sticking with us on this one. 🙂
Looks like it's just missing "@opentelemetry/resources": "1.14.0", in devDependencies and we can get this PR merged 🙂

[pichlermarc]

Thanks 🙂

[cristianmadularu]

Any idea when this fix will be available in a release?
Thanks

[pichlermarc]

Any idea when this fix will be available in a release? Thanks

Planning to release 0.41.0 next week - it will be in that one. 🙂

[cristianmadularu]

Perfect, thank you! :)

[jdt]

Any news on the 0.41.0 version this will be fixed in? I'm waiting on this one too and I'd like to see if there is anything I can do to help get it out there :-)

[pichlermarc]

@jdt still waiting for approvals for #3938 - it's the last PR that needs to be merged before we release 0.41.0.

[berktec]

coincidently we are awaiting this bug fix as well