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.

Sign up for GitHub

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

Jump to bottom

Manual Backport of [HCP Telemetry] Move first TelemetryConfig Fetch into the TelemetryConfigProvider into release/1.15.x #18627

Merged
merged 2 commits into from
Aug 31, 2023
Merged

Manual Backport of [HCP Telemetry] Move first TelemetryConfig Fetch into the TelemetryConfigProvider into release/1.15.x #18627

merged 2 commits into from
Aug 31, 2023

Conversation

Achooo
Copy link
Contributor

@Achooo Achooo commented Aug 31, 2023

Backport #18318 into release/1.15.x

The original backport PR had failed so I cherry picked the commit from main 0f48b7af5ec5fdf2237ba9e79a63160a071f6a91 here.

#18619

Description

Testing & Reproduction steps

Links

PR Checklist

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

@github-actions github-actions bot added the pr/dependencies PR specifically updates dependencies of project label Aug 31, 2023
@Achooo Achooo added pr/no-changelog PR does not need a corresponding .changelog entry pr/no-backport labels Aug 31, 2023
@Achooo Achooo mentioned this pull request Aug 31, 2023
Closed
4 tasks
@Achooo Achooo requested review from chapmanc and loshz August 31, 2023 16:01
@Achooo
Copy link
Contributor Author

Achooo commented Aug 31, 2023 •
edited
Loading

Not sure how to redeploy that Vercel failure, can't seem to login/ have access to hit the redeploy button.
Screenshot 2023-08-31 at 12 02 11 PM
Will try an empty commit to kick ci

Edit: that did not work :o

Think this needs to be fixed 🤔

Node.js Version "14.x" is discontinued and must be upgraded. Please set "engines": { "node": "18.x" } in your `package.json` file to use Node.js 18.

@Achooo
[HCP Telemetry] Move first TelemetryConfig Fetch into the TelemetryCo…
0ab3ba9 
…nfigProvider (#18318)

* Add Enabler interface to turn sink on/off

* Use h for hcpProviderImpl vars, fix PR feeback and fix errors

* Keep nil check in exporter and fix tests

* Clarify comment and fix function name

* Use disable instead of enable

* Fix errors nit in otlp_transform

* Add test for refreshInterval of updateConfig

* Add disabled field in MetricsConfig struct

* Fix PR feedback: improve comment and remove double colons

* Fix deps test which requires a maybe

* Update hcp-sdk-go to v0.61.0

* use disabled flag in telemetry_config.go

* Handle 4XX errors in telemetry_provider

* Fix deps test

* Check 4XX instead

* Run make go-mod-tidy
@Achooo Achooo force-pushed the ash-backport-0f48b7af5ec5fdf2237ba9e79a63160a071f6a91 branch from b026dd8 to 0ab3ba9 Compare August 31, 2023 16:04
@Achooo Achooo changed the title Backport of [HCP Telemetry] Move first TelemetryConfig Fetch into the TelemetryConfigProvider into release/1.15.x Manual Backport of [HCP Telemetry] Move first TelemetryConfig Fetch into the TelemetryConfigProvider into release/1.15.x Aug 31, 2023
@Achooo
Copy link
Contributor Author

Achooo commented Aug 31, 2023 •
edited
Loading

Oh I think its supposed to get skipped but this is broken 🤔 This is from a different PR that got back ported recently, and has the same failure : #18599
Screenshot 2023-08-31 at 12 12 04 PM

chapmanc
chapmanc reviewed Aug 31, 2023
View reviewed changes
test-integ/go.mod Outdated
@@ -0,0 +1,246 @@
module github.com/hashicorp/consul/test-integ
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this test-integ module for ? Also don't love a hypen in a package name https://go.dev/blog/package-names

Finally this is under consul which means it's a integration test for all of consul which I don't think you were intending to make? Let me know what you think.

Copy link
Contributor Author

@Achooo Achooo Aug 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh thanks - I need to remove this. I just cherry-picked the original commit. It's because main has these files (https://github.com/hashicorp/consul/tree/main/test-integ) so they got updated, but not this release branch.

I will remove these, good catch 💯

@Achooo Achooo force-pushed the ash-backport-0f48b7af5ec5fdf2237ba9e79a63160a071f6a91 branch from 39d746c to 8c1e907 Compare August 31, 2023 18:05
@Achooo Achooo requested a review from chapmanc August 31, 2023 18:38
@Achooo Achooo merged commit 4f4a95d into release/1.15.x Aug 31, 2023
@Achooo Achooo deleted the ash-backport-0f48b7af5ec5fdf2237ba9e79a63160a071f6a91 branch August 31, 2023 19:04
@jira jira bot mentioned this pull request Dec 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loshz loshz loshz approved these changes

@chapmanc chapmanc chapmanc approved these changes

Assignees
No one assigned
Labels
pr/dependencies PR specifically updates dependencies of project pr/no-backport pr/no-changelog PR does not need a corresponding .changelog entry
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Achooo @loshz @chapmanc