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

Chore: Add pkg/util/log package #5187

Merged
merged 3 commits into from
Jan 20, 2022
Merged

Chore: Add pkg/util/log package #5187

merged 3 commits into from
Jan 20, 2022

Conversation

aknuds1
Copy link
Contributor

@aknuds1 aknuds1 commented Jan 19, 2022 •
edited
Loading

What this PR does / why we need it:
Add pkg/util/log package, replacing github.com/cortexproject/cortex/pkg/util/log.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Checklist

  • [na] Documentation added
  • Tests updated
  • Add an entry in the CHANGELOG.md about the changes.

@aknuds1 aknuds1 force-pushed the chore/arve/drop-cortex-log branch 2 times, most recently from fd5a310 to 26622ca Compare January 19, 2022 17:45
@aknuds1 aknuds1 mentioned this pull request Jan 19, 2022
Closed
2 tasks
@aknuds1 aknuds1 marked this pull request as ready for review January 19, 2022 17:46
@aknuds1 aknuds1 requested a review from a team as a code owner January 19, 2022 17:46
@aknuds1 aknuds1 force-pushed the chore/arve/drop-cortex-log branch 6 times, most recently from 27679cb to 7b03913 Compare January 20, 2022 09:34
kavirajk
kavirajk approved these changes Jan 20, 2022
View reviewed changes
Copy link
Contributor

@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.

LGTM 👍 Thanks @aknuds1 Appreciate it.

clients/cmd/promtail/main.go
@@ -81,7 +81,7 @@ func main() {
fmt.Println("Invalid log level")
os.Exit(1)
}
util_log.InitLogger(&config.ServerConfig.Config)
util_log.InitLogger(&config.ServerConfig.Config, prometheus.DefaultRegisterer)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice :)

@aknuds1 aknuds1 force-pushed the chore/arve/drop-cortex-log branch from 7b03913 to d8a7741 Compare January 20, 2022 09:40
@aknuds1
Copy link
Contributor Author

aknuds1 commented Jan 20, 2022 •
edited
Loading

Thanks @kavirajk! I had to add this metric namespace in the meantime to avoid metric conflict (with Cortex I should think), that caused example config validation to fail (in CI). Does it look alright?

@aknuds1
Copy link
Contributor Author

aknuds1 commented Jan 20, 2022 •
edited
Loading

@kavirajk I am also wondering if I should note in the changelog that the names of two metrics change: loki_experimental_features_in_use_total and loki_log_messages_total?

sandeepsukhani
sandeepsukhani approved these changes Jan 20, 2022
View reviewed changes
Copy link
Contributor

@sandeepsukhani sandeepsukhani left a comment

Choose a reason for hiding this comment

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

Thanks Arve for taking care of it! LGTM!
I feel it is okay to break those two metrics and add an entry in the changelog since we do not have them in any of the dashboards.

@aknuds1
Copy link
Contributor Author

aknuds1 commented Jan 20, 2022

Thanks Arve for taking care of it! LGTM! I feel it is okay to break those two metrics and add an entry in the changelog since we do not have them in any of the dashboards.

@sandeepsukhani OK I'll add to the changelog!

@aknuds1 aknuds1 force-pushed the chore/arve/drop-cortex-log branch from 52d6549 to 95c95a1 Compare January 20, 2022 10:40
@aknuds1
Copy link
Contributor Author

aknuds1 commented Jan 20, 2022

@sandeepsukhani @kavirajk added to the changelog!

jeschkies
jeschkies reviewed Jan 20, 2022
View reviewed changes
Copy link
Contributor

@jeschkies jeschkies left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this!

CHANGELOG.md Show resolved Hide resolved
pkg/ingester/instance.go
@@ -8,6 +8,7 @@ import (
"syscall"

"github.com/cortexproject/cortex/pkg/cortexpb"
cutil "github.com/cortexproject/cortex/pkg/util"
Copy link
Contributor

Choose a reason for hiding this comment

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

How come this is kept?

Copy link
Contributor Author

@aknuds1 aknuds1 Jan 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@jeschkies the github.com/cortexproject/cortex/pkg/util package isn't part of the PR's topic though? Only github.com/cortexproject/cortex/pkg/util/log.

@aknuds1
Chore: Add pkg/util/log package
ba5ea44 
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@aknuds1
Add changelog entry
714dbcd 
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@aknuds1
Fix documentation of loki_log_messages_total
dfc5b73 
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@aknuds1 aknuds1 force-pushed the chore/arve/drop-cortex-log branch from 95c95a1 to dfc5b73 Compare January 20, 2022 17:22
@owen-d owen-d merged commit a2dd731 into main Jan 20, 2022
@owen-d owen-d deleted the chore/arve/drop-cortex-log branch January 20, 2022 17:53
@aknuds1
Copy link
Contributor Author

aknuds1 commented Jan 20, 2022

Thanks @owen-d!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kavirajk kavirajk kavirajk approved these changes

@jeschkies jeschkies jeschkies left review comments

@sandeepsukhani sandeepsukhani sandeepsukhani approved these changes

@KMiller-Grafana KMiller-Grafana Awaiting requested review from KMiller-Grafana

Assignees
No one assigned
Labels
size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aknuds1 @sandeepsukhani @jeschkies @kavirajk @owen-d