Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Remove LOG_ALL_GRAPHQL_REQUESTS env var.#43181

Merged
vrto merged 2 commits into
mainfrom
mv/auditlog/cleanup/graphql-log
Oct 24, 2022
Merged

Remove LOG_ALL_GRAPHQL_REQUESTS env var.#43181
vrto merged 2 commits into
mainfrom
mv/auditlog/cleanup/graphql-log

Conversation

@vrto

@vrto vrto commented Oct 19, 2022

Copy link
Copy Markdown
Contributor

Description

Remove LOG_ALL_GRAPHQL_REQUESTS env var. This is succeeded by log.auditLog.graphQL site config setting.

Test plan

  • tests pass
  • verified that the replacement (audit log) works as expected

This is succeeded by `log.auditLog.graphQl` site config setting.
@cla-bot cla-bot Bot added the cla-signed label Oct 19, 2022
@vrto vrto requested review from a team and rafax October 19, 2022 14:58

@rafax rafax left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Change LGTM, not sure if LOG_ALL_GRAPHQL_REQUESTS is still used by customers?

If so, would it make sense to leave a "you set LOG_ALL_GRAPHQL_REQUESTS, which was replaced by log.auditLog.graphQL" warning / panic (cost to maintainability of our code is minimal, but it can prevent surprises when customers no longer get audit logs)?

@vrto

vrto commented Oct 24, 2022

Copy link
Copy Markdown
Contributor Author

I talked w/ the team regarding how risky "simply dropping the var" is, and we agreed we'll go for it.

  • the env var was never promoted in the changelog
    • customers would need to talk to our team and/or read the source code to find out about it
  • the new changelog entry will mention that it is deprecated and suggests using the audit log instead
  • there are 0 or 1 (0 more likely) customers using this

@vrto vrto merged commit fea5117 into main Oct 24, 2022
@vrto vrto deleted the mv/auditlog/cleanup/graphql-log branch October 24, 2022 10:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants