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 logging when we fail to export metrics to hcp #20514

Merged
merged 3 commits into from
Feb 8, 2024

Conversation

jjti
Copy link
Contributor

@jjti jjti commented Feb 6, 2024

Description

Testing 1.18.x I noticed some stderr log lines w/o the normal hclog formatting:

2024-02-06T22:35:18.972Z [ERROR] agent.envoy: Error receiving new DeltaDiscoveryRequest; closing request channel: error="rpc error: code = Canceled desc = context canceled"
2024-02-06T22:35:19.072Z [ERROR] agent.envoy: Error receiving new DeltaDiscoveryRequest; closing request channel: error="rpc error: code = Canceled desc = context canceled"
2024/02/06 22:35:31 failed to export metrics: failed to export metrics: code 404: 404 page not found
2024/02/06 22:44:31 failed to export metrics: failed to export metrics: code 404: 404 page not found

Here in OTEL there's a GlobalHandler that defaults to logging to stderr: https://github.com/open-telemetry/opentelemetry-go/blob/main/internal/global/handler.go#L96

The OTEL PeriodicReader calls that here whenever otelExporter.Export fails: https://github.com/open-telemetry/opentelemetry-go/blob/e3eb3f7538e790a853c3ce210cf48123ddd5ca20/sdk/metric/periodic_reader.go#L181

This change here is to override the default global error handler with one that logs to the hclog.Logger instead

Testing & Reproduction steps

Links

PR Checklist

  • updated test coverage
  • external facing docs updated
  • appropriate backport labels added
  • not a security concern

@jjti jjti added backport/1.17 This release series is no longer active on CE. Use backport/ent/1.17. backport/1.18 labels Feb 6, 2024
@jjti jjti requested a review from Achooo February 6, 2024 23:12
@jjti jjti requested a review from loshz February 6, 2024 23:16
@Achooo
Copy link
Contributor

Achooo commented Feb 7, 2024

Just a callout for future reviewers, or anyone looking back at this PR: it's a global level logger, this would override the logging for any other future code using the OTEL library since the logger is set here. Ideally, we can scope our logger down to metrics if the metrics SDK permits , but not sure if that's currently possible💯. Will leave @jjti to do this digging and make the right call here. LGTM.

@hc-github-team-consul-core
Copy link
Collaborator

@jjti, a backport is missing for this PR [20514] for versions [1.17] please perform the backport manually and add the following snippet to your backport PR description:

<details>
	<summary> Overview of commits </summary>
		- <<backport commit 1>>
		- <<backport commit 2>>
		...
</details>

3 similar comments
@hc-github-team-consul-core
Copy link
Collaborator

@jjti, a backport is missing for this PR [20514] for versions [1.17] please perform the backport manually and add the following snippet to your backport PR description:

<details>
	<summary> Overview of commits </summary>
		- <<backport commit 1>>
		- <<backport commit 2>>
		...
</details>

@hc-github-team-consul-core
Copy link
Collaborator

@jjti, a backport is missing for this PR [20514] for versions [1.17] please perform the backport manually and add the following snippet to your backport PR description:

<details>
	<summary> Overview of commits </summary>
		- <<backport commit 1>>
		- <<backport commit 2>>
		...
</details>

@hc-github-team-consul-core
Copy link
Collaborator

@jjti, a backport is missing for this PR [20514] for versions [1.17] please perform the backport manually and add the following snippet to your backport PR description:

<details>
	<summary> Overview of commits </summary>
		- <<backport commit 1>>
		- <<backport commit 2>>
		...
</details>

@loshz loshz removed the backport/1.17 This release series is no longer active on CE. Use backport/ent/1.17. label Feb 12, 2024
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.

4 participants