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

Implement unmarshal traces with jsoniter, make 35x faster than jsonpb(#4817) #4986

Conversation

hanjm
Copy link
Member

@hanjm hanjm commented Mar 13, 2022

Description:

As the #4817 and the trace proto are stable, I implement unmarshal traces with jsoniter iterator parser, compare with jsonpb unmarshaller, jsoniter does not use reflect, more efficient, less GC pressure.
The benchmark results show 35x faster, 5x fewer allocs, 19x fewer alloc bytes.

$ go test -bench "Traces(J|P)"
goos: darwin
goarch: amd64
pkg: go.opentelemetry.io/collector/model/otlp
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkTracesJSONUnmarshal-12            10000            107938 ns/op           83057 B/op       1228 allocs/op
BenchmarkTracesJSONiterUnmarshal-12       427092              2768 ns/op            4314 B/op        206 allocs/op

If it is acceptable. i think we can make JSON unmarshaller is configurable, jsonpb is default, jsoniter is optional.
I want to split the full function into a series of small PR:

  1. Implement trace unmarshaller.
  2. Implement logs unmarshaller.
  3. Implement metrics unmarshaller.
  4. Make theotlpreceiver support config jsoniter as optional unmarshaller.

Link to tracking Issue:
#4817
Testing: < Describe what testing was performed and which tests were added.>

  1. unit test. construct a trace message, fill all the fields, use jsonpb marshal to json bytes, use jsoniter unmarshal to struct, test assert there are equal.

@hanjm hanjm requested a review from a team as a code owner March 13, 2022 02:31
@hanjm hanjm requested a review from dmitryax March 13, 2022 02:31
@hanjm hanjm force-pushed the featur/jimmiehan-add-jsoniter-unmarshaller branch from bfa3fcd to 6ff4ba6 Compare March 13, 2022 02:42
@codecov
Copy link

codecov bot commented Mar 13, 2022

Codecov Report

Merging #4986 (1e46b4d) into main (041f398) will increase coverage by 0.22%.
The diff coverage is 98.98%.

@@            Coverage Diff             @@
##             main    #4986      +/-   ##
==========================================
+ Coverage   90.53%   90.76%   +0.22%     
==========================================
  Files         190      190              
  Lines       11111    11407     +296     
==========================================
+ Hits        10059    10353     +294     
- Misses        829      833       +4     
+ Partials      223      221       -2     
Impacted Files Coverage Δ
pdata/ptrace/json.go 97.15% <98.98%> (+12.15%) ⬆️
pdata/internal/common.go 95.01% <0.00%> (+0.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 041f398...1e46b4d. Read the comment docs.

@hanjm hanjm force-pushed the featur/jimmiehan-add-jsoniter-unmarshaller branch 2 times, most recently from f4bfa8b to 0ec4ace Compare March 13, 2022 02:51
@hanjm hanjm changed the title Implement unmarshal traces with jsoniter, make 40x faster (#4817) Implement unmarshal traces with jsoniter, make 40x faster than jsonpb(#4817) Mar 13, 2022
@hanjm hanjm force-pushed the featur/jimmiehan-add-jsoniter-unmarshaller branch from 0ec4ace to a47dea8 Compare March 13, 2022 03:06
@bogdandrutu
Copy link
Member

We try to discourage JSON usage because of performance impact in general anyway. What is the use case for using JSON instead of proto encoded?

@hanjm
Copy link
Member Author

hanjm commented Mar 13, 2022

We try to discourage JSON usage because of performance impact in general anyway. What is the use case for using JSON instead of proto encoded?

Yes, I know JSON performance is lower than proto, but in some use case, JSON is more friendly to frontend(web/mobile) or Lua script runtime to report telemetry data. protobuf has extra generated code dependency.

I add a benchmark compare to jsoniter vs protobuf. jsoniter is only 2.x slower pb. a lot of improvements compare to jsonpb.

BenchmarkTracesJSONiterUnmarshal-12       493048              2324 ns/op            4001 B/op        185 allocs/op
BenchmarkTracesPBUnmarshal-12            1000000              1049 ns/op            2816 B/op         78 allocs/op

@hanjm
Copy link
Member Author

hanjm commented Mar 25, 2022

@hanjm hanjm force-pushed the featur/jimmiehan-add-jsoniter-unmarshaller branch 2 times, most recently from 01dbf3e to c568091 Compare March 25, 2022 10:16
@hanjm hanjm changed the title Implement unmarshal traces with jsoniter, make 40x faster than jsonpb(#4817) Implement unmarshal traces with jsoniter, make 35x faster than jsonpb(#4817) Mar 25, 2022
@hanjm
Copy link
Member Author

hanjm commented Mar 28, 2022

We save 250 CPU cores in production after applying the optimized OTLP JSON jsoniter parser.
image

@bogdandrutu
Copy link
Member

@hanjm sorry for the wait. I am waiting for the pdata split to finish first before making more changes in that package.

@bogdandrutu
Copy link
Member

Please rebase.

@hanjm hanjm force-pushed the featur/jimmiehan-add-jsoniter-unmarshaller branch 2 times, most recently from 31effc1 to e6d317e Compare April 21, 2022 03:39
@hanjm
Copy link
Member Author

hanjm commented Apr 21, 2022

Rebase Done.
but the "contrib-tests" CI fail

# github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/opencensus
pkg/translator/opencensus/metrics_to_oc.go:369:3: invalid case pmetric.MetricValueTypeInt in switch on exemplar.ValueType() (mismatched types "go.opentelemetry.io/collector/pdata/internal".NumberDataPointValueType and "go.opentelemetry.io/collector/pdata/internal".ExemplarValueType)
pkg/translator/opencensus/metrics_to_oc.go:371:3: invalid case pmetric.MetricValueTypeDouble in switch on exemplar.ValueType() (mismatched types "go.opentelemetry.io/collector/pdata/internal".NumberDataPointValueType and "go.opentelemetry.io/collector/pdata/internal".ExemplarValueType)
FAIL	github.com/open-telemetry/opentelemetry-collector-contrib/internal/components [build failed]

@dmitryax
Copy link
Member

contrib-tests failure is expected. It will be fixed once open-telemetry/opentelemetry-collector-contrib#9389 is merged

@hanjm hanjm force-pushed the featur/jimmiehan-add-jsoniter-unmarshaller branch from e6d317e to f954620 Compare April 22, 2022 02:10
@hanjm
Copy link
Member Author

hanjm commented Apr 22, 2022

@bogdandrutu Rebase done, Please help review, thank you.

pdata/ptrace/jsoniter.go Outdated Show resolved Hide resolved
pdata/ptrace/jsoniter.go Outdated Show resolved Hide resolved
pdata/ptrace/jsoniter.go Outdated Show resolved Hide resolved
@hanjm hanjm force-pushed the featur/jimmiehan-add-jsoniter-unmarshaller branch from d68f650 to ac572a4 Compare May 1, 2022 14:32
@hanjm hanjm force-pushed the featur/jimmiehan-add-jsoniter-unmarshaller branch from ac572a4 to 8ece8e7 Compare May 2, 2022 07:48
@hanjm hanjm requested a review from bogdandrutu May 2, 2022 08:17
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.

None yet

3 participants