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

promtail: Handle nil error on target Details() call #7771

Merged
merged 4 commits into from
Nov 30, 2022
Merged

promtail: Handle nil error on target Details() call #7771

merged 4 commits into from
Nov 30, 2022

Conversation

GeorgeTsilias
Copy link
Contributor

What this PR does / why we need it:
This PR addresses issue #6930.
Promtail should not panic when the Details() method is called for a target that has no errors.

Which issue(s) this PR fixes:
Fixes #6930

Special notes for your reviewer:
This is my first contribution, please let me know if I need to add more details regarding the small fix.

Checklist

  • Reviewed the CONTRIBUTING.md guide
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

@GeorgeTsilias GeorgeTsilias requested a review from a team as a code owner November 24, 2022 17:16
@CLAassistant
Copy link

CLAassistant commented Nov 24, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@GeorgeTsilias GeorgeTsilias mentioned this pull request Nov 24, 2022
Closed
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

chaudum
chaudum approved these changes Nov 28, 2022
View reviewed changes
Copy link
Contributor

@chaudum chaudum left a comment

Choose a reason for hiding this comment

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

Hey @GeorgeTsilias Thanks for the contribution! LGTM

@chaudum
Copy link
Contributor

chaudum commented Nov 28, 2022

@GeorgeTsilias Can you also add a changelog entry (under Promtail / Fixes)?

@chaudum
Copy link
Contributor

chaudum commented Nov 29, 2022

@GeorgeTsilias If you want, I can add the changelog entry for you and then merge it.

@GeorgeTsilias
Copy link
Contributor Author

Hey @chaudum , I'll be available to dig around and add the entry in about 3-4 hours. If you're in a hurry (due to an upcoming release or something) feel free to do it up!
Thanks for the responses and sorry for being late to answer!

@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

@chaudum chaudum merged commit d3615f7 into grafana:main Nov 30, 2022
@chaudum
Copy link
Contributor

chaudum commented Nov 30, 2022

@GeorgeTsilias Thanks for your contribution!

Abuelodelanada pushed a commit to canonical/loki that referenced this pull request Dec 1, 2022
@GeorgeTsilias @Abuelodelanada
promtail: Handle nil error on target Details() call (grafana#7771)
898f55c 
**What this PR does / why we need it**:
This PR addresses issue [grafana#6930](grafana#6930).
Promtail should not panic when the Details() method is called for a target that has no errors.

**Which issue(s) this PR fixes**:
Fixes grafana#6930
@tpaschalis tpaschalis mentioned this pull request Dec 8, 2022
Closed
@DylanGuedes DylanGuedes added type/bug Somehing is not working as expected backport release-2.7.x add to a PR to backport it into release 2.7.x labels Feb 23, 2023
@grafanabot grafanabot mentioned this pull request Feb 23, 2023
Merged
grafanabot pushed a commit that referenced this pull request Feb 23, 2023
@GeorgeTsilias @grafanabot
promtail: Handle nil error on target Details() call (#7771)
9c6a004 
**What this PR does / why we need it**:
This PR addresses issue [#6930](#6930).
Promtail should not panic when the Details() method is called for a target that has no errors.

**Which issue(s) this PR fixes**:
Fixes #6930

(cherry picked from commit d3615f7)
DylanGuedes added a commit that referenced this pull request Feb 23, 2023
@grafanabot @GeorgeTsilias @DylanGuedes
[release-2.7.x] promtail: Handle nil error on target Details() call (#…
356c2c3 
…8604)

Backport d3615f7 from #7771

---------

Co-authored-by: George Tsilias <tsiliasg@gmail.com>
Co-authored-by: Dylan Guedes <djmgguedes@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chaudum chaudum chaudum approved these changes

Assignees
No one assigned
Labels
backport release-2.7.x add to a PR to backport it into release 2.7.x size/S type/bug Somehing is not working as expected
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

promtail: panic when fetching target Details
5 participants
@GeorgeTsilias @CLAassistant @grafanabot @chaudum @DylanGuedes