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

promtail: no need for GCP promtail_instance label now that loki supports out-of-order writes #4556

Merged

Conversation

james-callahan
Copy link
Contributor

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #4474

Special notes for your reviewer:

Checklist

  • Documentation updated
  • Tests updated

@CLAassistant
Copy link

CLAassistant commented Oct 26, 2021

CLA assistant check
All committers have signed the CLA.

@owen-d
Copy link
Member

owen-d commented Oct 26, 2021

Great catch. Are you willing to sign the CLA?

@james-callahan james-callahan force-pushed the remove-gcp-promtail_instance-label branch from f301cff to 0b6ae2a Compare October 26, 2021 16:24
@james-callahan
Copy link
Contributor Author

Great catch. Are you willing to sign the CLA?

Done

Copy link
Collaborator

@kavirajk kavirajk left a comment

Choose a reason for hiding this comment

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

Thanks @james-callahan. LGTM!.

@owen-d this could be a breaking change. should be add it to the upgrade guide?

@owen-d
Copy link
Member

owen-d commented Nov 1, 2021

Good call @kavirajk. Let's add a small note to the upgrade guide mentioning the lost label :)

@owen-d
Copy link
Member

owen-d commented Nov 1, 2021

It also looks like we'll need one small change to pass the linter:

clients/pkg/promtail/targets/gcplog/formatter.go:21:5: var `instanceID` is unused (unused)
186s
19	var instanceID uuid.UUID

@james-callahan
Copy link
Contributor Author

It also looks like we'll need one small change to pass the linter

Fixed + rebased.

The tests now seem to be failing for unrelated reasons?

@owen-d
Copy link
Member

owen-d commented Nov 3, 2021

Hey, the reason it's failing is because by removing some code, it's actually changed the expected go.mod configs because the uuid library is now an indirect dependency:

-	github.com/gofrs/uuid v4.0.0+incompatible
+	github.com/gofrs/uuid v4.0.0+incompatible // indirect

You should be able to resolve this by running go mod tidy and committing after.

@james-callahan james-callahan force-pushed the remove-gcp-promtail_instance-label branch from ca691cc to 6b94342 Compare November 3, 2021 02:04
@james-callahan
Copy link
Contributor Author

Hey, the reason it's failing is because by removing some code, it's actually changed the expected go.mod configs because the uuid library is now an indirect dependency:

huh. weird that this manifested so far away in the tests (only failures in compactor retention tests); while the lint stage passed.

You should be able to resolve this by running go mod tidy and committing after.

Done.

@owen-d owen-d merged commit a92541b into grafana:main Nov 3, 2021
kavirajk added a commit that referenced this pull request Nov 3, 2021
Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
owen-d pushed a commit that referenced this pull request Nov 3, 2021
Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove gcplog prometheus_instance label
4 participants