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

Use grpc client config from cortex for Ingester to get more control #755

Conversation

sandeepsukhani
Copy link
Contributor

⚠️ Breaking change in the parsing of flags since max_recv_msg_size is moved.

Ignoring the use of gzip compression flag and turning it ON always by default like how we were doing already without the flag. The current value of it is false in Cortex, do we want to honour it? If we do its default value should ideally be true and there is no clean way to change the default without changing code in cortex project.

grpc.WithDefaultCallOptions(
grpc.MaxCallRecvMsgSize(cfg.MaxRecvMsgSize),
grpc.UseCompressor("gzip"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this be overwritten by cfg.GRPCClientConfig.DialOption() to false by default ?

Copy link
Contributor

Choose a reason for hiding this comment

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

A test would make me happy here to prove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It won't be overwritten. We are setting defaults which can be overwritten while invoking the grpc call.
cfg.GRPCClientConfig.DialOption is also setting default options.

grpc.WithDefaultCallOptions(cfg.CallOptions()...),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually connection level default which you can overwriting during each grpc call.

Copy link
Contributor

Choose a reason for hiding this comment

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

both set grpc.WithDefaultCallOptions through DialOption. I'm not so sure about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

The option array here has 2 grpc.WithDefaultCallOptions one with gzip one without.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ho I get it ! the second on will just not add the compressor. the first on adds it. ok.

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

@cyriltovena
Copy link
Contributor

I let you squash and merge that one, I think you need to update also our cluster.

@@ -20,7 +20,9 @@
},

ingester_client_config: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gouthamve @Kuqd I guess this does not get applied, needs to be moved in loki block down below with name ingester_client. This does not match config schema

IngesterClient client.Config `yaml:"ingester_client,omitempty"`

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch yes !

@stale
Copy link

stale bot commented Sep 3, 2019

This issue has been automatically marked as stale because it has not had any activity in the past 30 days. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale A stale issue or PR that will automatically be closed. label Sep 3, 2019
@sandeepsukhani sandeepsukhani added the keepalive An issue or PR that will be kept alive and never marked as stale. label Sep 9, 2019
@stale stale bot removed the stale A stale issue or PR that will automatically be closed. label Sep 9, 2019
@sandeepsukhani sandeepsukhani force-pushed the ingester-client-config-from-cortex branch from 61e7612 to be3dea6 Compare September 10, 2019 12:23
@sandeepsukhani sandeepsukhani merged commit 107dfd3 into grafana:master Sep 16, 2019
rfratto added a commit to rfratto/loki that referenced this pull request Sep 25, 2019
cyriltovena pushed a commit that referenced this pull request Sep 25, 2019
* docs: create structure of docs overhaul

This commit removes all old docs and lays out the table of contents and
framework for how the new documentation will be intended to be read.

* docs: add design docs back in

* docs: add community documentation

* docs: add LogQL docs

* docs: port existing operations documentation

* docs: add new placeholder file for promtail configuration docs

* docs: add TOC for operations/storage

* docs: add Loki API documentation

* docs: port troubleshooting document

* docs: add docker-driver documentation

* docs: link to configuration from main docker-driver document

* docs: update API for new paths

* docs: fix broken links in api.md and remove json marker from examples

* docs: incorporate api changes from #1009

* docs: port promtail documentation

* docs: add TOC to promtail configuration reference

* docs: fix promtail spelling errors

* docs: add loki configuration reference

* docs: add TOC to configuration

* docs: add loki configuration example

* docs: add Loki overview with brief explanation about each component

* docs: add comparisons document

* docs: add info on table manager and update storage/README.md

* docs: add getting started

* docs: incorporate config yaml changes from #755

* docs: fix typo in releases url for promtail

* docs: add installation instructions

* docs: add more configuration examples

* docs: add information on fluentd client

fluent-bit has been temporarily removed until the PR for it is merged.

* docs: PR review feedback

* docs: add architecture document

* docs: add missing information from old docs

* `localy` typo

Co-Authored-By: Ed Welch <[email protected]>

* docs: s/ran/run/g

* Typo

* Typo

* Tyop

* Typo

* docs: fixed typo

* docs: PR feedback

* docs: @cyriltovena PR feedback

* docs: add more details to promtail url config option

* docs: expand promtail's pipelines document with extra detail

* docs: remove reference to Stage interface in pipelines.md

* docs: fixed some spelling

* docs: clarify promtail configuration and scraping

* docs: attempt #2 at explaining promtail's usage of machine hostname

* docs: spelling fixes

* docs: add reference to promtail custom metrics and fix silly typo

* docs: cognizant -> aware

* docs: typo

* docs: typos

* docs: add which components expose which API endpoints in microservices mode

* docs: change ksonnet installation to tanka

* docs: address most @pracucci feedback

* docs: fix all spelling errors so reviewers don't have to keep finding them :)

* docs: incorporate changes to API endpoints made in #1022

* docs: add missing loki metrics

* docs: add missing promtail metrics

* docs: @pstribrany feedback

* docs: more @pracucci feedback

* docs: move metrics into a table

* docs: update push path references to /loki/api/v1/push

* docs: add detail to further explain limitations of monolithic mode

* docs: add alternative names to modes_of_operation diagram

* docs: add log ordering requirement

* docs: add procedure for updating docs with latest version

* docs: separate out stages documentation into one document per stage

* docs: list supported stores in storage documentation

* docs: add info on duplicate log lines in pipelines

* docs: add line_format as key feature to fluentd

* docs: hopefully final commit :)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keepalive An issue or PR that will be kept alive and never marked as stale.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants