Implement unmarshal metrics with jsoniter #5433

atingchen · 2022-05-26T14:12:53Z

Description:
I implement unmarshal metrics with jsoniter iterator parser, compare with jsonpb unmarshaller, jsoniter does not use reflect, more efficient, less GC pressure.

Link to tracking Issue: [#4986 ]
Testing:
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.

linux-foundation-easycla · 2022-05-26T14:12:56Z

The committers listed above are authorized under a signed CLA.

✅ login: atingchen / name: look (fb1b1de, 0fbed74, 6b32d70, d0c9a50)

hanjm

💪
General LGTM. Just two small nits.

hanjm · 2022-05-26T23:54:41Z

pdata/pmetric/json.go

+	return kv
+}
+
+func readAnyValue(iter *jsoniter.Iterator, f string) otlpcommon.AnyValue {


This common function could be move to some place. https://github.com/open-telemetry/opentelemetry-collector/tree/main/pdata/internal

I agree, but I'm not sure where it's appropriate to move it。
There seems to be no suitable place under package internal

@bogdandrutu @dmitryax Could you help review, thank you.

Let's use a pdata/internal/json

hanjm · 2022-05-27T00:07:04Z

pdata/pmetric/json_test.go

+	if assert.Error(t, iter.Error) {
+		assert.Contains(t, iter.Error.Error(), "unknown field")
+	}
+}


It will be better to add benchmark compare to jsonpb.

I will do it

github-actions · 2022-06-16T03:37:10Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

hanjm · 2022-06-16T05:03:07Z

still valid

codecov · 2022-06-22T14:04:35Z

Codecov Report

Merging #5433 (417c99d) into main (9e90e25) will increase coverage by 0.28%.
The diff coverage is 98.47%.

@@            Coverage Diff             @@
##             main    #5433      +/-   ##
==========================================
+ Coverage   91.44%   91.72%   +0.28%     
==========================================
  Files         196      197       +1     
  Lines       11958    12475     +517     
==========================================
+ Hits        10935    11443     +508     
- Misses        808      816       +8     
- Partials      215      216       +1

Impacted Files	Coverage Δ
pdata/pmetric/json.go	`97.94% <98.11%> (-2.06%)`	⬇️
pdata/internal/json/common.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.

tigrannajaryan · 2022-06-22T14:10:39Z

@atingchen thank you for the PR. I started reviewing it but I see that the build is failing. Can you please fix the build to make sure all github actions pass so that we can then review the PR. Faster performance is great, however we need to make there is very thorough coverage by the tests since this deals with data received externally.

atingchen · 2022-06-22T14:23:19Z

@tigrannajaryan Thank you for your comments. I will fix it as soon as possible

linux-foundation-easycla · 2022-07-10T08:31:00Z

The committers listed above are authorized under a signed CLA.

✅ login: atingchen / name: look (2a3b60f, 0bb9c1c, f67bd70, 5045e6d, ff7fc03, 6d8da86)

tigrannajaryan · 2022-07-11T14:23:11Z

Please make sure EasyCLA passes and there are no irrelevant changes.

atingchen · 2022-07-12T11:29:20Z

It is caused by the error of executing the command merge, and it has been solved now

tigrannajaryan · 2022-07-14T16:23:55Z

Please make sure the build passes. Preferably you should do this before creating a PR. make with various targets can be run locally.

pdata/pmetric/json.go

pdata/pmetric/json_test.go

tigrannajaryan · 2022-07-20T19:16:53Z

@open-telemetry/collector-approvers please review. Question: do we need any additional precautions to ensure we did not break OTLP/JSON compatibility or the tests that are introduced are good enough?

pdata/pmetric/json_test.go

Aneurysm9 · 2022-07-26T16:05:08Z

CHANGELOG.md

@@ -20,6 +20,7 @@
 - Expose `pcommon.NewSliceFromRaw` function (#5679)
 - `loggingexporter`: create the exporter's logger from the service's logger (#5677)
 - Add `otelcol_exporter_queue_capacity` metrics show the collector's exporter queue capacity (#5475)
+- Add `jsoniter` Unmarshaller (#5433)


This should be more specific. Where is jsoniter used? Is it added or replacing something? It looks like this only applies to metrics, is there a reason for that?

Compare with jsonpb unmarshaller, jsoniter does not use reflect, more efficient, less GC pressure.
The ChangeLog will be more specific.

It looks like this only applies to metrics, is there a reason for that?

Traces were done earlier: #4986

Aneurysm9 · 2022-07-26T16:28:10Z

@open-telemetry/collector-approvers please review. Question: do we need any additional precautions to ensure we did not break OTLP/JSON compatibility or the tests that are introduced are good enough?

Is it possible to maintain the current implementation somewhere and add tests that demonstrate that the same JSON documents are unmarshalled to equivalent pdata structures by both implementations? I think that would give me the most confidence in the new implementation. It would also allow us to introduce this change with a feature gate, which would allow users who ran into issues we didn't test for to quickly revert to the old implementation.

atingchen · 2022-07-26T16:38:09Z

Is it possible to maintain the current implementation somewhere and add tests that demonstrate that the same JSON documents are unmarshalled to equivalent pdata structures by both implementations? I think that would give me the most confidence in the new implementation. It would also allow us to introduce this change with a feature gate, which would allow users who ran into issues we didn't test for to quickly revert to the old implementation.

Thanks for your suggestion, I'll think about how to implement it

bogdandrutu · 2022-08-08T17:17:56Z

CHANGELOG.md

@@ -126,7 +126,7 @@ There isn't a valid core binary for this release. Use v0.57.2 instead.
 - Expose `pcommon.NewSliceFromRaw` function (#5679)
 - `loggingexporter`: create the exporter's logger from the service's logger (#5677)
 - Add `otelcol_exporter_queue_capacity` metrics show the collector's exporter queue capacity (#5475)
- Add support to handle 402, 413, 414, 431 http error code as permanent errors in OTLP exporter (#5685)
+- Add support to handle 402, 413, 414, 431 http error code as permanent errors in OTLP exporter (#5685) 


Suggested change

- Add support to handle 402, 413, 414, 431 http error code as permanent errors in OTLP exporter (#5685)

- Add support to handle 402, 413, 414, 431 http error code as permanent errors in OTLP exporter (#5685)

Ping on this

pdata/pmetric/json.go

bogdandrutu · 2022-08-08T17:26:07Z

pdata/pmetric/json.go

+	return kv
+}
+
+func readAnyValue(iter *jsoniter.Iterator, f string) otlpcommon.AnyValue {


Let's use a pdata/internal/json

bogdandrutu

Fix lint issues

internal/json/common.go:21: File is not `goimports`-ed with -local go.opentelemetry.io/collector (goimports)
	jsoniter "github.com/json-iterator/go"
internal/json/common_test.go:21: File is not `goimports`-ed with -local go.opentelemetry.io/collector (goimports)
	"github.com/stretchr/testify/assert"

pdata/internal/json/common.go

… options.

…inition

…etrics. 2. Some functions move to pdata/internal/json 3.Ignore unknown fields and have a debug log for them

atingchen requested review from a team and tigrannajaryan May 26, 2022 14:12

atingchen mentioned this pull request May 26, 2022

[feature request] performance: otlp http json support jsoniter/sonic for better performance #4817

Closed

hanjm reviewed May 27, 2022

View reviewed changes

github-actions bot added the Stale label Jun 16, 2022

github-actions bot removed the Stale label Jun 17, 2022

atingchen closed this Jun 22, 2022

atingchen reopened this Jun 22, 2022

atingchen force-pushed the feature/lookchen-add-jsoniter-unmarshaller branch 2 times, most recently from 6d8da86 to 9a5b5a0 Compare July 12, 2022 11:27

tigrannajaryan reviewed Jul 14, 2022

View reviewed changes

pdata/pmetric/json.go Outdated Show resolved Hide resolved

pdata/pmetric/json_test.go Outdated Show resolved Hide resolved

atingchen force-pushed the feature/lookchen-add-jsoniter-unmarshaller branch from 9a5b5a0 to 6860a53 Compare July 17, 2022 09:38

tigrannajaryan reviewed Jul 20, 2022

View reviewed changes

pdata/pmetric/json_test.go Show resolved Hide resolved

atingchen force-pushed the feature/lookchen-add-jsoniter-unmarshaller branch from 5aa8020 to dec0094 Compare July 23, 2022 08:37

Aneurysm9 reviewed Jul 26, 2022

View reviewed changes

atingchen force-pushed the feature/lookchen-add-jsoniter-unmarshaller branch from dec0094 to 9c97c4b Compare August 6, 2022 04:44

bogdandrutu reviewed Aug 8, 2022

View reviewed changes

bogdandrutu reviewed Aug 15, 2022

View reviewed changes

pdata/internal/json/common.go Show resolved Hide resolved

atingchen added 18 commits August 16, 2022 10:29

Implement unmarshal metrics with jsoniter

8099d3c

Implement unmarshal metrics with jsoniter AND add test file

8ea52a4

add unit testing

45a78c9

add unit testing

ebb280b

git rebase main

9f2e594

do a matrix testing for all 8 cases for a combination of these 3 bool…

43dd5f1

… options.

some func is deprecated

500a49c

go imports

54254e4

go imports

48ef29d

Update change log

fb509e4

generate the 8 cases per tested func to avoid repeating test case def…

1e9b2c3

…inition

1. Remove instrumentationLibraryMetrics and instrumentation_library_m…

bf70e70

…etrics. 2. Some functions move to pdata/internal/json 3.Ignore unknown fields and have a debug log for them

add test unit

7b9489e

update CHANGELOG

168d6fc

remove the new API AND reuse the other API NewJSONUnmarshaler

61ce01f

update CHANGELOG

71e7b5e

update CHANGELOG

0872d61

go mod tidy

ac4a9b9

atingchen force-pushed the feature/lookchen-add-jsoniter-unmarshaller branch from 6416178 to ac4a9b9 Compare August 16, 2022 02:46

remove unnecessary return true on the default branch

417c99d

bogdandrutu merged commit ada07a5 into open-telemetry:main Aug 16, 2022

bogdandrutu mentioned this pull request Aug 17, 2022

Use common json parsing code for attributes #5923

Merged

atingchen deleted the feature/lookchen-add-jsoniter-unmarshaller branch September 6, 2022 10:23

atingchen mentioned this pull request Sep 6, 2022

Make the otlpreceiver support to use jsoniter to unmarshal JSON Document. #6027

Closed

mx-psi mentioned this pull request Sep 14, 2022

[docs] Update release instructions #6068

Merged

atingchen mentioned this pull request Sep 19, 2022

REQUEST: New membership for @atingchen open-telemetry/community#1195

Closed

6 tasks

atingchen mentioned this pull request Sep 27, 2022

[receiver/otlpjsonfile] Excessive errors created when consuming multi-signal files open-telemetry/opentelemetry-collector-contrib#12603

Closed

This was referenced Sep 28, 2022

[receiver/otlp] Rewrite logs related to server startup #6174

Merged

[pdata] Update deprecation comments #6206

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement unmarshal metrics with jsoniter #5433

Implement unmarshal metrics with jsoniter #5433

atingchen commented May 26, 2022

linux-foundation-easycla bot commented May 26, 2022 •

edited

Loading

hanjm left a comment

hanjm May 26, 2022

atingchen Jun 1, 2022

hanjm Jun 1, 2022

bogdandrutu Aug 8, 2022

hanjm May 27, 2022

atingchen May 28, 2022

github-actions bot commented Jun 16, 2022

hanjm commented Jun 16, 2022

codecov bot commented Jun 22, 2022 •

edited

Loading

tigrannajaryan commented Jun 22, 2022

atingchen commented Jun 22, 2022

linux-foundation-easycla bot commented Jul 10, 2022 •

edited

Loading

tigrannajaryan commented Jul 11, 2022

atingchen commented Jul 12, 2022

tigrannajaryan commented Jul 14, 2022

tigrannajaryan commented Jul 20, 2022

Aneurysm9 Jul 26, 2022

atingchen Jul 26, 2022 •

edited

Loading

tigrannajaryan Jul 26, 2022

Aneurysm9 commented Jul 26, 2022

atingchen commented Jul 26, 2022

bogdandrutu Aug 8, 2022

bogdandrutu Aug 12, 2022

bogdandrutu Aug 8, 2022

bogdandrutu left a comment

	- Add support to handle 402, 413, 414, 431 http error code as permanent errors in OTLP exporter (#5685)
	- Add support to handle 402, 413, 414, 431 http error code as permanent errors in OTLP exporter (#5685)

Implement unmarshal metrics with jsoniter #5433

Implement unmarshal metrics with jsoniter #5433

Conversation

atingchen commented May 26, 2022

linux-foundation-easycla bot commented May 26, 2022 • edited Loading

hanjm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 16, 2022

hanjm commented Jun 16, 2022

codecov bot commented Jun 22, 2022 • edited Loading

Codecov Report

tigrannajaryan commented Jun 22, 2022

atingchen commented Jun 22, 2022

linux-foundation-easycla bot commented Jul 10, 2022 • edited Loading

tigrannajaryan commented Jul 11, 2022

atingchen commented Jul 12, 2022

tigrannajaryan commented Jul 14, 2022

tigrannajaryan commented Jul 20, 2022

Choose a reason for hiding this comment

atingchen Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aneurysm9 commented Jul 26, 2022

atingchen commented Jul 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdandrutu left a comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented May 26, 2022 •

edited

Loading

codecov bot commented Jun 22, 2022 •

edited

Loading

linux-foundation-easycla bot commented Jul 10, 2022 •

edited

Loading

atingchen Jul 26, 2022 •

edited

Loading