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

Added Query Logging proposal #2694

Merged
merged 13 commits into from
Jul 28, 2020
Merged

Added Query Logging proposal #2694

merged 13 commits into from
Jul 28, 2020

Conversation

yashrsharma44
Copy link
Contributor

@yashrsharma44 yashrsharma44 commented Jun 1, 2020 •
edited
Loading

Signed-off-by: Yash yashrsharma44@gmail.com

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

@bwplotka @kakkoyun

@yashrsharma44 yashrsharma44 marked this pull request as draft June 1, 2020 05:47
bwplotka
bwplotka reviewed Jun 1, 2020
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

uu, awesome! Will take a look before our meeting 🤗 We have busy week (F2F)

kakkoyun
kakkoyun reviewed Jun 4, 2020
View reviewed changes
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

I just skimmed but it looks good so far.

What I'd prefer is to split the document into two.
As we talked before let's focus on query logging and put the limiting aside for now. We can create another proposal for that.

docs/proposals/202005_query_logging_rate_limiting.md Outdated Show resolved Hide resolved
bwplotka
bwplotka reviewed Jun 4, 2020
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Nice! Some comments during our meeting.

docs/proposals/202005_query_logging_rate_limiting.md Outdated Show resolved Hide resolved
docs/proposals/202005_query_logging_rate_limiting.md Outdated Show resolved Hide resolved
docs/proposals/202005_query_logging_rate_limiting.md Outdated Show resolved Hide resolved
docs/proposals/202005_query_logging_rate_limiting.md Outdated Show resolved Hide resolved
docs/proposals/202005_query_logging_rate_limiting.md Outdated Show resolved Hide resolved
docs/proposals/202005_query_logging_rate_limiting.md Outdated Show resolved Hide resolved
docs/proposals/202005_query_logging_rate_limiting.md Outdated Show resolved Hide resolved
@yashrsharma44 yashrsharma44 changed the title Added Query Logging and limiting proposal Added Query Logging proposal Jun 8, 2020
kakkoyun
kakkoyun reviewed Jun 9, 2020
View reviewed changes
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

Good job, looking good.

A section that I'd like to see is how to correlate logs from different components? How to track the logs of an individual query between different component?

And concerning audit logs; do we have a strategy to deal with sensitive data?

docs/proposals/202005_query_logging.md Outdated Show resolved Hide resolved
docs/proposals/202005_query_logging.md Outdated Show resolved Hide resolved
docs/proposals/202005_query_logging.md Outdated Show resolved Hide resolved
docs/proposals/202005_query_logging.md Show resolved Hide resolved
@yashrsharma44
Copy link
Contributor Author

yashrsharma44 commented Jun 9, 2020 •
edited
Loading

Good job, looking good.

A section that I'd like to see is how to correlate logs from different components? How to track the logs of an individual query between different component?

I haven't checked out if tracing uses an id for reference, otherwise, we can reuse the tracing id in the logger, so that we can track the request across different components, wdyt?
Anyway, we can use ULID as a request id, so that we can track the request across the different components.
The current tracer configuration passes the tracer-id for a given request as a metadata, so we can re-use it as a request-id, so that we can identify a given request in a log, passing through various components.

I would add a section regarding this shortly.

bwplotka
bwplotka reviewed Jun 25, 2020
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Some comments during our offline meetings.

docs/proposals/202005_query_logging.md Show resolved Hide resolved
docs/proposals/202005_query_logging.md Show resolved Hide resolved
docs/proposals/202005_query_logging.md Show resolved Hide resolved
docs/proposals/202005_query_logging.md Outdated Show resolved Hide resolved
@yashrsharma44 yashrsharma44 marked this pull request as ready for review June 25, 2020 12:39
@yashrsharma44 yashrsharma44 mentioned this pull request Jun 27, 2020
Closed
@yashrsharma44 yashrsharma44 mentioned this pull request Jul 6, 2020
Closed
kakkoyun
kakkoyun reviewed Jul 9, 2020
View reviewed changes
docs/proposals/202005_query_logging.md Outdated Show resolved Hide resolved
docs/proposals/202005_query_logging.md Outdated Show resolved Hide resolved
docs/proposals/202005_query_logging.md Show resolved Hide resolved
docs/proposals/202005_query_logging.md Outdated Show resolved Hide resolved
docs/proposals/202005_query_logging.md Outdated Show resolved Hide resolved
docs/proposals/202005_query_logging.md Show resolved Hide resolved
docs/proposals/202005_query_logging.md Show resolved Hide resolved
bwplotka
bwplotka reviewed Jul 9, 2020
View reviewed changes
docs/proposals/202005_query_logging.md Outdated Show resolved Hide resolved
docs/proposals/202005_query_logging.md Outdated Show resolved Hide resolved
docs/proposals/202005_query_logging.md Outdated Show resolved Hide resolved
bwplotka
bwplotka reviewed Jul 22, 2020
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

I believe we should get this done and approved, and merged before getting into actual implementation (: This will avoid confusion we had in last middleware PRs (:

We can always iterate on this 💪

@yashrsharma44
Copy link
Contributor Author

yashrsharma44 commented Jul 22, 2020 •
edited
Loading

Test failure unrelated -

--- FAIL: TestReloader_ConfigApply (0.62s)
    reloader_test.go:148: reloader_test.go:148:
        	exp: 2
        	got: 3
FAIL
FAIL	github.com/thanos-io/thanos/pkg/reloader	2.145s

Edit :
Rebased with master to fix the changes.

@yashrsharma44
Added Query Logging and limiting proposal
c93ec2d 
Signed-off-by: Yash <yashrsharma44@gmail.com>
@yashrsharma44
fix spelling mistakes
52ba883 
Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
@yashrsharma44
Modified the proposal to add mentors requirements
3b3f0b2 
Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
@yashrsharma44
Renamed file and added non-goals
9865e38 
Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
@yashrsharma44
Formatting
bf99518 
Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
@yashrsharma44
Added some more additions
99bdb9b 
Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
@yashrsharma44
Add common usecase
c1d8e10 
Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
@yashrsharma44
Add log meta
a766ab4 
Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
@yashrsharma44
Added middleware implementation
909acfb 
Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
@yashrsharma44
added some details about how request logging could be used by the user
ce20cd2 
Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
@yashrsharma44
added policy for request logging
70e9658 
Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
@yashrsharma44
nitpickings
d27177c 
Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
@yashrsharma44
added more suggestions based on reviewers
035b116 
Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
kakkoyun
kakkoyun approved these changes Jul 28, 2020
View reviewed changes
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

🥇 LGMT. Let's merge it. If we discover any issues we can amend it later on.

@yashrsharma44
Copy link
Contributor Author

Yayy!!

@kakkoyun kakkoyun merged commit 8d62d42 into thanos-io:master Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwplotka bwplotka bwplotka left review comments

@kakkoyun kakkoyun kakkoyun approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yashrsharma44 @kakkoyun @bwplotka