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 json trace/metrics unmarshaling for numbers #5924

Merged
merged 1 commit into from
Aug 17, 2022

Conversation

bogdandrutu
Copy link
Member

@bogdandrutu bogdandrutu commented Aug 16, 2022

Updates #5312

Also this improves benchmarks since ReadAny was doing an allocation, see:

Before:

/private/var/folders/5c/5p_3jmvd6qx0rsvmb9j7c_s00000gn/T/GoLand/___BenchmarkJSONUnmarshal_in_go_opentelemetry_io_collector_pdata_ptrace.test -test.v -test.paniconexit0 -test.bench ^\QBenchmarkJSONUnmarshal\E$ -test.run ^$
goos: darwin
goarch: amd64
pkg: go.opentelemetry.io/collector/pdata/ptrace
cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
BenchmarkJSONUnmarshal
BenchmarkJSONUnmarshal-16    	  334974	      3942 ns/op	    4669 B/op	     231 allocs/op
PASS

After:

/private/var/folders/5c/5p_3jmvd6qx0rsvmb9j7c_s00000gn/T/GoLand/___BenchmarkJSONUnmarshal_in_go_opentelemetry_io_collector_pdata_ptrace.test -test.v -test.paniconexit0 -test.bench ^\QBenchmarkJSONUnmarshal\E$ -test.run ^$
goos: darwin
goarch: amd64
pkg: go.opentelemetry.io/collector/pdata/ptrace
cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
BenchmarkJSONUnmarshal
BenchmarkJSONUnmarshal-16    	  374025	      3642 ns/op	    4412 B/op	     219 allocs/op
PASS

Signed-off-by: Bogdan <[email protected]>

@bogdandrutu bogdandrutu requested review from a team and tigrannajaryan August 16, 2022 21:20
@bogdandrutu bogdandrutu force-pushed the fixjsonintparce branch 4 times, most recently from 407107d to 769a475 Compare August 16, 2022 21:29
@codecov
Copy link

codecov bot commented Aug 16, 2022

Codecov Report

Merging #5924 (61b0c60) into main (d8407ff) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5924      +/-   ##
==========================================
+ Coverage   91.80%   91.84%   +0.03%     
==========================================
  Files         198      198              
  Lines       12377    12429      +52     
==========================================
+ Hits        11363    11415      +52     
  Misses        800      800              
  Partials      214      214              
Impacted Files Coverage Δ
pdata/internal/json/number.go 100.00% <100.00%> (ø)
pdata/pmetric/json.go 97.94% <100.00%> (ø)
pdata/ptrace/json.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@bogdandrutu bogdandrutu force-pushed the fixjsonintparce branch 2 times, most recently from b4b1b48 to c000949 Compare August 16, 2022 22:15
@tigrannajaryan
Copy link
Member

Github shows lots of line changes but it seems many of the changes are just moving the code without changing functionality. Can you summarize the changes (other than performance improvement)?

@bogdandrutu
Copy link
Member Author

bogdandrutu commented Aug 17, 2022

@tigrannajaryan good point, split the first simple PR #5926 then will split one more time to have focus changes.

A set of the changes are in the CHANGELOG, one thing I don't like about having a CHANGELOG is that you will ask me to copy same thing in the contrib description :)))

pdata/internal/json/enum.go Outdated Show resolved Hide resolved
pdata/internal/json/number.go Outdated Show resolved Hide resolved
@bogdandrutu bogdandrutu force-pushed the fixjsonintparce branch 3 times, most recently from 696f5e2 to 67163d4 Compare August 17, 2022 15:49
@bogdandrutu bogdandrutu changed the title Fix json unmarshaling for numbers and enums Fix json trace/metrics unmarshaling for numbers Aug 17, 2022
- Accept both string and number for int32/uint32.
- Read uint64 numbers without converting from int64.

Signed-off-by: Bogdan <[email protected]>
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.

3 participants