Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: otlp json encoding #4220

Merged
merged 5 commits into from
Nov 6, 2023
Merged

Conversation

seemk
Copy link
Contributor

@seemk seemk commented Oct 22, 2023

Which problem is this PR solving?

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #4216

Short description of the changes

Fixes the breakage introduced in #4062 where OTLP JSON encoding was encoding the intermediate low and high bytes directly to JSON, but in JSON mapping fixed64 can only be either a string or a number.

Removed usage of the parts taken from long.js and used BigInt instead with a fallback to the old worse precision hrTimeToNanoseconds from core if BigInt is not available.

Refactored the createExport{Trace,Metrics,Logs}ServiceRequest calls to unified options and an encoder (getOtlpEncoder), added tests for the encoder.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Tested manually against collector with gRPC, http/proto, http/json exporters.

Checklist:

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

@codecov
Copy link

codecov bot commented Oct 22, 2023

Codecov Report

Merging #4220 (c0145d8) into main (586def4) will decrease coverage by 1.71%.
The diff coverage is 66.66%.

❗ Current head c0145d8 differs from pull request most recent head fe78777. Consider uploading reports for the commit fe78777 to get more accurate results

@@            Coverage Diff             @@
##             main    #4220      +/-   ##
==========================================
- Coverage   92.24%   90.54%   -1.71%     
==========================================
  Files         331      159     -172     
  Lines        9481     3765    -5716     
  Branches     1999      837    -1162     
==========================================
- Hits         8746     3409    -5337     
+ Misses        735      356     -379     
Files Coverage Δ
...tlp-http/src/platform/browser/OTLPTraceExporter.ts 100.00% <100.00%> (ø)
...lp-http/src/platform/browser/OTLPMetricExporter.ts 100.00% <100.00%> (ø)
...-otlp-http/src/platform/browser/OTLPLogExporter.ts 83.33% <0.00%> (ø)

... and 175 files with indirect coverage changes

@evantorrie
Copy link

evantorrie commented Nov 1, 2023

but in JSON mapping fixed64 can only be either a string or a number.

That linked table seems to indicate that fixed64 is preferred to be a string, whereas fixed32 is preferred to be a number (although either would be accepted). I see that this PR does output the fixed64 as a string, so seems good to me.

@trentm
Copy link
Contributor

trentm commented Nov 2, 2023

It would be helpful if this branch could be updated to the latest in "main". That should hopefully fix the "Unit Tests / ..." check failures, now that #4238 has gone in.

@evantorrie
Copy link

Is there any way to expedite getting this patch in?

For some users, the last release (1.17.1 and the experimental packages 0.44.0) is broken. Web-based JSON exporters in this release broke by sending data that does not conform to the JSON serialization of the protobufs expected by the OpenTelemetry Collector.

That release was on October 10, so we are getting close to a month now where we have a broken release as the "latest release".

Could we please get this fix merged and a new release (1.17.2? 0.44.1?) pushed ASAP?

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the test faliures that were fixed by #4238 held this PR up quite a bit while I was out of office 😞

I ran some local tests with the exporter example and the changes look good to me. Thank you @seemk for taking care of this.

I'll push a commit to this PR to move the changelog entry - then I'll merge; I'll continue to review PRs in this repo for the rest of my day and then I'll open a release PR so we can get this published as soon as possible.

@pichlermarc pichlermarc merged commit 9db6352 into open-telemetry:main Nov 6, 2023
17 checks passed
dyladan added a commit that referenced this pull request Nov 15, 2023
* chore: track package-lock.json (#4238)

* chore: track package-lock.json

* Pin to old versions for node 14

* Use version range

* Remove unused cached directories

* Temporarily disable other tests

* Temporarily enable only api test

* Enable only some packages

* Test only api packages

* Test trace exporters

* Fix line ordering

* Test all packages except otlp exporters

* Add trace http exporter

* Add trace proto exporter

* Test all but grpc exporters

* chore: use npm workspaces and degrade lerna to v6

* chore: get rid of lerna bootstrap

* chore: use npx

* chore: allow install scripts to setup buf

* chore: fix w3c-integration-test cache key

* chore: fix cache key

* chore: disable resource compat test

* chore: fix node_modules assumptions

* chore: fix hoisted karma issue

* chore: fix markdown linter complaints

* chore: lock @grpc/grpc-js to v1.8.21

* Break caches

* chore: remove cache

* chore: fixup inline commands

---------

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>

* docs: fixed link to benchmark results (#4233)

Co-authored-by: Chengzhong Wu <legendecas@gmail.com>

* chore(deps): update all patch versions (#4215)

* fix: otlp json encoding (#4220)

Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>

* fix: remove duplicate export star from version.ts (#4225)

Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>

* docs: fix sdk-node config instructions (#4249)

Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>

* feat(api): publish api esnext target (#4231)

* chore: release API 1.7.0/Core 1.18.0/Experimental 0.45.0 (#4254)

* fix(sdk-metrics): hand-roll MetricAdvice type as older API versions do not include it (#4260)

* chore: prepare release 1.18.1/0.45.1 (#4261)

* chore: no need for 'packages' in "lerna.json" (#4264)

* Benchmark tests for trace OTLP transform and BatchSpanProcessor (#4218)

Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>

* chore: type reference on zone.js (#4257)

Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>

* docs: add docker-compose to run prometheus for the experimental example (#4268)

Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>

* fix(sdk-logs): avoid map attribute set when count limit exceeded (#4195)

Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>

* chore(deps): update dependency chromedriver to v119 [security] (#4280)

* chore(deps): update actions/setup-node action to v4 (#4236)

* fix(sdk-trace-base): processor onStart called with a span having empty attributes (#4277)

Co-authored-by: artahmetaj <artahmetaj@yahoo.com>

* Update fetch instrumentation to be runtime agnostic (#4063)

Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>

---------

Co-authored-by: Chengzhong Wu <legendecas@gmail.com>
Co-authored-by: Martin Kuba <martin@martinkuba.com>
Co-authored-by: Mend Renovate <bot@renovateapp.com>
Co-authored-by: Siim Kallas <siimkallas@gmail.com>
Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
Co-authored-by: David Luna <david.luna@elastic.co>
Co-authored-by: Dinko Osrecki <dinko.osrecki@emarsys.com>
Co-authored-by: Trent Mick <trentm@gmail.com>
Co-authored-by: François <32224751+Lp-Francois@users.noreply.github.com>
Co-authored-by: Hyun Oh <hyunnoh01@gmail.com>
Co-authored-by: André Cruz <andremiguelcruz@msn.com>
Co-authored-by: artahmetaj <artahmetaj@yahoo.com>
Co-authored-by: drewcorlin1 <82601620+drewcorlin1@users.noreply.github.com>
gdaszuta added a commit to gdaszuta/otel-cf-workers that referenced this pull request Aug 21, 2024
Regression introduced in commit evanderkoogh@bd0a46a after api change in open-telemetry/opentelemetry-js#4220
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@opentelemetry/exporter-trace-otlp-http sends time as an object
4 participants