fix errcheck issue for aws related exporter #9785

fatsheep9146 · 2022-05-07T01:16:06Z

Signed-off-by: Ziqi Zhao zhaoziqi9146@gmail.com

Description:
fix errcheck and gocritic issue for aws related exporter, awscloudwatchlogsexporter, awsemfexporter, awsxrayexporter

Link to tracking Issue:
#9749 #10466

fatsheep9146 · 2022-05-07T15:25:27Z

@mx-psi Please help review this pr, thanks :)

fatsheep9146 · 2022-05-10T15:46:38Z

@Aneurysm9 Can you help review this pr?

pmm-sumo · 2022-05-11T15:05:06Z

I think this should have changelog entry

mx-psi

Left a couple of comments, but I think we should wait for @Aneurysm9 to be back and have a look at these

mx-psi · 2022-05-11T15:20:31Z

exporter/awscloudwatchlogsexporter/exporter.go

-	expConfig.Validate()
+	err = expConfig.Validate()
+	if err != nil {
+		return nil, err
+	}


This can go away entirely, validation is already done by the service so this will never fail.

The "service" you mention is cwlogs.NewClient fucntion?

No, it's the one on opentelemetry-collector. The call happens here https://github.com/open-telemetry/opentelemetry-collector/blob/c56d20e9e0afccc43179da12f60e24e5729caca9/service/config_provider.go#L117, which validates the configuration of all components https://github.com/open-telemetry/opentelemetry-collector/blob/c56d20e9e0afccc43179da12f60e24e5729caca9/config/moved_config.go#L53

Yes, this validation should be able to be removed. It seems overly defensive given that the service hosts will already have validated it.

mx-psi · 2022-05-11T15:20:44Z

exporter/awsemfexporter/emf_exporter.go

-	expConfig.Validate()
+	err = expConfig.Validate()
+	if err != nil {
+		return nil, err
+	}


mx-psi · 2022-05-11T15:21:57Z

exporter/awsxrayexporter/internal/translator/cause.go

@@ -181,7 +180,7 @@ func fillJavaStacktrace(stacktrace string, exceptions []awsxray.Exception) []aws
 	r := textproto.NewReader(bufio.NewReader(strings.NewReader(stacktrace)))

 	// Skip first line containing top level exception / message
-	r.ReadLine()
+	_, _ = r.ReadLine()


Should we check the error and return exceptions here, the same way it's done when lines are actually read?

Yes, probably. An error here would seem to indicate a failure that should stop further processing.

This still needs to be addressed

github-actions · 2022-06-07T05:20:20Z

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

exporter/awsxrayexporter/internal/translator/writer_pool_test.go

exporter/awsxrayexporter/awsxray_test.go

fatsheep9146 · 2022-06-08T05:44:42Z

@Aneurysm9 @mx-psi I've already fix comments and fix the gocritic failed cases in the mean time.

djaglowski · 2022-06-15T14:22:31Z

This appears to need a make gotidy and possibly a rebase as well.

mx-psi · 2022-06-15T14:42:30Z

exporter/awscloudwatchlogsexporter/exporter.go

+	err = expConfig.Validate()
+	if err != nil {
+		return nil, err
+	}


This still needs to be removed

mx-psi · 2022-06-15T14:43:44Z

exporter/awsxrayexporter/internal/translator/cause.go

@@ -181,7 +180,7 @@ func fillJavaStacktrace(stacktrace string, exceptions []awsxray.Exception) []aws
 	r := textproto.NewReader(bufio.NewReader(strings.NewReader(stacktrace)))

 	// Skip first line containing top level exception / message
-	r.ReadLine()
+	_, _ = r.ReadLine()


This still needs to be addressed

fatsheep9146 · 2022-06-16T12:21:09Z

ping @mx-psi , please help review this again, thanks. :)

exporter/awscloudwatchlogsexporter/exporter.go

exporter/awsemfexporter/emf_exporter.go

mx-psi

LGTM, but would like @Aneurysm9 to have a final look too

djaglowski · 2022-06-17T12:49:16Z

There are some unit tests failing: https://github.com/open-telemetry/opentelemetry-collector-contrib/runs/6918563670?check_suite_focus=true#step:7:202

FAIL github.com/open-telemetry/opentelemetry-collector-contrib/exporter/awsemfexporter

djaglowski

Please resolve unit test failures

fatsheep9146 · 2022-06-19T12:26:37Z

Please resolve unit test failures

Ok.

Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com>

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>

Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com>

Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>

Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com>

fatsheep9146 · 2022-06-19T14:26:07Z

@djaglowski unit test is fixed, but the check-links failed, but I checked that the failed issues are not related to aws related exporters, should I resolve those in another pr?

djaglowski · 2022-06-20T14:45:44Z

Thanks @fatsheep9146. I agree the failure is unrelated. (I've opened #11167 to address it.)

fatsheep9146 requested a review from a team May 7, 2022 01:16

fatsheep9146 requested a review from Aneurysm9 as a code owner May 7, 2022 01:16

github-actions bot assigned codeboten May 7, 2022

bogdandrutu assigned Aneurysm9 and unassigned codeboten May 9, 2022

mx-psi reviewed May 11, 2022

View reviewed changes

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

Aneurysm9 removed the Stale label Jun 7, 2022

Aneurysm9 reviewed Jun 7, 2022

View reviewed changes

exporter/awsxrayexporter/internal/translator/writer_pool_test.go Outdated Show resolved Hide resolved

Aneurysm9 reviewed Jun 7, 2022

View reviewed changes

exporter/awsxrayexporter/awsxray_test.go Outdated Show resolved Hide resolved

fatsheep9146 force-pushed the fatsheep9146/fix-errcheck-failed-issue branch from 9857e69 to 248b459 Compare June 8, 2022 05:43

mx-psi reviewed Jun 15, 2022

View reviewed changes

fatsheep9146 force-pushed the fatsheep9146/fix-errcheck-failed-issue branch from 248b459 to f362869 Compare June 16, 2022 10:33

mx-psi reviewed Jun 16, 2022

View reviewed changes

exporter/awscloudwatchlogsexporter/exporter.go Outdated Show resolved Hide resolved

exporter/awsemfexporter/emf_exporter.go Outdated Show resolved Hide resolved

mx-psi added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jun 17, 2022

mx-psi approved these changes Jun 17, 2022

View reviewed changes

djaglowski requested changes Jun 17, 2022

View reviewed changes

fatsheep9146 and others added 4 commits June 19, 2022 21:03

fix errcheck issue for aws related exporter

75bc67d

Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com>

Update exporter/awsxrayexporter/internal/translator/writer_pool_test.go

2428a78

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>

Update exporter/awsxrayexporter/awsxray_test.go

5083696

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>

fix comments and gocritic error

eec3127

Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com>

fatsheep9146 and others added 3 commits June 19, 2022 21:03

fix for reviews

3041684

Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com>

Apply suggestions from code review

f4fbf29

Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>

fix failed unit test

130bc28

Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com>

fatsheep9146 force-pushed the fatsheep9146/fix-errcheck-failed-issue branch from 91fed70 to 130bc28 Compare June 19, 2022 13:03

fatsheep9146 added 2 commits June 19, 2022 21:15

fix unit test failed

5f5c3fc

Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com>

fix unit test failed

830a4f9

Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com>

djaglowski approved these changes Jun 20, 2022

View reviewed changes

djaglowski merged commit 426c5a9 into open-telemetry:main Jun 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix errcheck issue for aws related exporter #9785

fix errcheck issue for aws related exporter #9785

fatsheep9146 commented May 7, 2022 •

edited

Loading

fatsheep9146 commented May 7, 2022

fatsheep9146 commented May 10, 2022

pmm-sumo commented May 11, 2022

mx-psi left a comment

mx-psi May 11, 2022

fatsheep9146 May 14, 2022

mx-psi May 23, 2022

Aneurysm9 Jun 7, 2022

mx-psi May 11, 2022

mx-psi May 11, 2022

Aneurysm9 Jun 7, 2022

mx-psi Jun 15, 2022

github-actions bot commented Jun 7, 2022

fatsheep9146 commented Jun 8, 2022

djaglowski commented Jun 15, 2022

mx-psi Jun 15, 2022

mx-psi Jun 15, 2022

fatsheep9146 commented Jun 16, 2022 •

edited

Loading

mx-psi left a comment

djaglowski commented Jun 17, 2022

djaglowski left a comment

fatsheep9146 commented Jun 19, 2022

fatsheep9146 commented Jun 19, 2022

djaglowski commented Jun 20, 2022

fix errcheck issue for aws related exporter #9785

fix errcheck issue for aws related exporter #9785

Conversation

fatsheep9146 commented May 7, 2022 • edited Loading

fatsheep9146 commented May 7, 2022

fatsheep9146 commented May 10, 2022

pmm-sumo commented May 11, 2022

mx-psi 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 7, 2022

fatsheep9146 commented Jun 8, 2022

djaglowski commented Jun 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fatsheep9146 commented Jun 16, 2022 • edited Loading

mx-psi left a comment

Choose a reason for hiding this comment

djaglowski commented Jun 17, 2022

djaglowski left a comment

Choose a reason for hiding this comment

fatsheep9146 commented Jun 19, 2022

fatsheep9146 commented Jun 19, 2022

djaglowski commented Jun 20, 2022

fatsheep9146 commented May 7, 2022 •

edited

Loading

fatsheep9146 commented Jun 16, 2022 •

edited

Loading