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

Added support for tail to query frontend #2032

Merged
merged 9 commits into from
Jul 24, 2020
Merged

Conversation

tivvit
Copy link
Contributor

@tivvit tivvit commented May 2, 2020

What this PR does / why we need it:

  • It is adding support for tailing to query frontend (basically WebSocket proxy)

Which issue(s) this PR fixes:
First I wanted to create issue for this. Then I checked in the code and saw that it is not implemented and decided to try to implement it.

Special notes for your reviewer:

  • I am not sure how to handle client certificates, any ideas?
  • Is WsTailProxy in the correct place?

Checklist

  • Documentation added
  • Tests updated
  • I think it is expected behavior - no need to add documentation
  • Not sure how to test this

@codecov-io
Copy link

Codecov Report

Merging #2032 into master will decrease coverage by 0.03%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2032      +/-   ##
==========================================
- Coverage   64.46%   64.43%   -0.04%     
==========================================
  Files         131      131              
  Lines       10038    10046       +8     
==========================================
+ Hits         6471     6473       +2     
- Misses       3084     3091       +7     
+ Partials      483      482       -1     
Impacted Files Coverage Δ
pkg/loki/modules.go 15.25% <0.00%> (-0.54%) ⬇️
pkg/promtail/targets/tailer.go 78.40% <0.00%> (+2.27%) ⬆️

@cyriltovena
Copy link
Contributor

We skip the frontend on our side for tail request, but I'm not against this, although this means support will be limited.

WDYT @slim-bean ? If we're good I'll review this !

@tivvit
Copy link
Contributor Author

tivvit commented May 6, 2020

Hi @cyriltovena thanks for your reply. I think it is good to have direct support for this in the query frontend. It simplifies the setup when you are not using a proxy. I get that you can route the traffic based on path directly to the querier or to query frontend.

Yes the support is limited, but the features we are losing are related to auth. And when you are using auth you already have to use some auth proxy, therefore, it is not an issue anymore. Am I correct? But it definitely should be described in the docs somehow.

@tivvit
Copy link
Contributor Author

tivvit commented May 26, 2020

@cyriltovena did you have time to look at it?

@cyriltovena
Copy link
Contributor

cyriltovena commented May 26, 2020

I'm waiting for feedback from someone else, I'm not against it at all !

/cc @slim-bean @owen-d @sandeepsukhani

@sandeepsukhani sandeepsukhani self-requested a review May 26, 2020 15:21
pkg/loki/modules.go Outdated Show resolved Hide resolved
pkg/loki/modules.go Outdated Show resolved Hide resolved
@cyriltovena
Copy link
Contributor

I'll bring this up in the next meeting see if what the team thinks about it.

@periklis
Copy link
Collaborator

Good intent here! Thank you for contributing this. It goes a step closer to have simpler Loki deployments. E.g. on kubernetes, where you need to maintain three distinct services one for writes to the distributor, one for reads to the frontend and one for tails to the queriers. However, I ack to maintainers' comments, that a simple reverse proxy would fix the need to have the tail endpoint on the proxy. Thanks again for bringing this up and all the effort!

@cyriltovena
Copy link
Contributor

We don’t see any issues with this if you replace your implementation with this https://golang.org/pkg/net/http/httputil/#ReverseProxy since go1.12 it supports websockets.

🙏

@owen-d
Copy link
Member

owen-d commented Jun 19, 2020

Agree with @cyriltovena. I think we'll be able to get this merged.

@stale
Copy link

stale bot commented Jul 19, 2020

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 Jul 19, 2020
@tivvit
Copy link
Contributor Author

tivvit commented Jul 19, 2020

Not stale. I will prepare it with the use of the Reverseproxy from httputil

@stale stale bot removed the stale A stale issue or PR that will automatically be closed. label Jul 19, 2020
@sandeepsukhani sandeepsukhani added the keepalive An issue or PR that will be kept alive and never marked as stale. label Jul 20, 2020
@pull-request-size pull-request-size bot added size/M and removed size/L labels Jul 21, 2020
@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2020

Codecov Report

Merging #2032 into master will decrease coverage by 0.07%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2032      +/-   ##
==========================================
- Coverage   61.67%   61.60%   -0.08%     
==========================================
  Files         160      160              
  Lines       13569    13582      +13     
==========================================
- Hits         8369     8367       -2     
- Misses       4577     4592      +15     
  Partials      623      623              
Impacted Files Coverage Δ
pkg/loki/loki.go 0.00% <ø> (ø)
pkg/loki/modules.go 8.59% <0.00%> (-0.54%) ⬇️
pkg/querier/queryrange/downstreamer.go 95.87% <0.00%> (-2.07%) ⬇️
pkg/logql/evaluator.go 92.47% <0.00%> (-0.41%) ⬇️
pkg/promtail/targets/file/tailer.go 78.40% <0.00%> (+2.27%) ⬆️

@tivvit
Copy link
Contributor Author

tivvit commented Jul 21, 2020

it is working

I am not sure if the tailproxy config is in correct location.

Can somebody please guide me how to add section to the flags? Because now it is located in the main section.

@slim-bean
Copy link
Collaborator

Thanks for making these changes, will take a look today or tomorrow!

pkg/loki/modules.go Outdated Show resolved Hide resolved
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

@@ -308,7 +320,7 @@ func (t *Loki) initQueryFrontend() (_ services.Service, err error) {
t.server.HTTP.Handle("/api/prom/label/{name}/values", frontendHandler)
t.server.HTTP.Handle("/api/prom/series", frontendHandler)
// fallback route
t.server.HTTP.PathPrefix("/").Handler(frontendHandler)
t.server.HTTP.PathPrefix("/").Handler(tailProxy)
Copy link
Contributor

Choose a reason for hiding this comment

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

The default value for tailproxy.downstream-url is empty string and not everyone would want to set it but here we use it anyways. I think we should use it only when a value is set for 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.

oh great point I will fix 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.

great point thanks, I will fix 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.

what do you think about this solution ef61f93?

Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

This is looking good! A little nitpick here, then LGTM.

pkg/loki/loki.go Outdated
@@ -55,6 +56,7 @@ type Config struct {
TableManager chunk.TableManagerConfig `yaml:"table_manager,omitempty"`
Worker frontend.WorkerConfig `yaml:"frontend_worker,omitempty"`
Frontend frontend.Config `yaml:"frontend,omitempty"`
TailProxy tailproxy.Config `yaml:"tail_proxy,omitempty"`
Copy link
Member

@owen-d owen-d Jul 23, 2020

Choose a reason for hiding this comment

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

In addition to what @sandeepsukhani mentioned, I think this should be part of the frontend config rather than it's own top level value because it's only used in the frontend.

We may need to extend our frontend config definition to be something like

type Config struct {
    frontend.Config `yaml:",inline"`
    tail_proxy_url string `yaml:"tail_proxy_url"`
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good idea. I did it like this because I knew it does not belong to Cortex.

This is better I will prepare it

Copy link
Member

Choose a reason for hiding this comment

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

edit: i love the podracer gif :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@owen-d what do think about it now? I used embedded struct as you proposed and IMHO it is better.

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 cyriltovena merged commit 2e1fd42 into grafana:master Jul 24, 2020
cyriltovena pushed a commit to cyriltovena/loki that referenced this pull request Jun 11, 2021
* don't exec create keyspace if it exists

Signed-off-by: Wing924 <weihe924stephen@gmail.com>

* fix

Signed-off-by: Wing924 <weihe924stephen@gmail.com>

* fix

Signed-off-by: Wing924 <weihe924stephen@gmail.com>
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. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants