Skip to content
This repository has been archived by the owner on Mar 27, 2021. It is now read-only.

Enforce Mandatory Client ID (Github Issue #704) #727

Merged
merged 36 commits into from
Jan 26, 2021

Conversation

sming
Copy link
Contributor

@sming sming commented Nov 20, 2020

Enforce Mandatory Client ID (Issue #704 was issue PRISM-2191)

Use Case Resolved:

  • I am a Heroic developer
  • who wants to know who is querying the Heroic API
  • so that when Heroic's performance suffers due to egregious query patterns, we know who to contact

Design & Implementation Notes

  • See [RFC] Enforce Mandatory Client ID for detailed coverage of the Use Case outlined above.
  • but in a nutshell, we opted for Envoy but it didn't have the features necessary.
  • hence we reverted to using Java request filters in the Heroic API codebase.

The Deployment Plan

From Plan of Action :

  1. Send a warning email to all of engineering that says uses have 1 month to start setting Client ID to something accountable
  2. Simultaneously, send the same message to #monitoring-users
  3. after 1 month, send the messages again
  4. push to canaries and try out some non-compliant and compliant requests to check it’s working
  5. Mop up any lingering questions/problems on #monitoring-users
  6. deploy to all heroic instances
  7. field any issues that arise

@sming sming self-assigned this Nov 20, 2020
@sming sming linked an issue Nov 20, 2020 that may be closed by this pull request
working on unit tests
@sming
Copy link
Contributor Author

sming commented Jan 18, 2021

Reopening because we're "pivoting" (yet again ¯_(ツ)_/¯) back to doing this filtering in Heroic.

@sming sming reopened this Jan 18, 2021
@sming sming changed the title Enforce Mandatory Client ID (Github Issue #704) DRAFT - DO NOT REVIEW YET - Enforce Mandatory Client ID (Github Issue #704) Jan 18, 2021
@codecov
Copy link

codecov bot commented Jan 19, 2021

Codecov Report

Merging #727 (e333355) into master (3597263) will increase coverage by 0.00%.
The diff coverage is 76.47%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #727   +/-   ##
=========================================
  Coverage     55.00%   55.01%           
- Complexity     3148     3153    +5     
=========================================
  Files           746      747    +1     
  Lines         20377    20388   +11     
  Branches       1336     1337    +1     
=========================================
+ Hits          11208    11216    +8     
- Misses         8681     8684    +3     
  Partials        488      488           
Impacted Files Coverage Δ Complexity Δ
.../com/spotify/heroic/aggregation/simple/Module.java 78.46% <ø> (ø) 2.00 <0.00> (ø)
...spotify/heroic/consumer/pubsub/PubSubConsumer.java 50.00% <ø> (ø) 2.00 <0.00> (ø)
...com/spotify/heroic/aggregation/AbstractBucket.java 14.28% <ø> (ø) 1.00 <0.00> (ø)
...java/com/spotify/heroic/aggregation/AnyBucket.java 72.72% <ø> (ø) 4.00 <0.00> (ø)
...fy/heroic/metric/DistributionPointDeserialize.java 84.61% <ø> (ø) 3.00 <0.00> (ø)
.../com/spotify/heroic/metric/HeroicDistribution.java 100.00% <ø> (ø) 3.00 <0.00> (ø)
...ain/java/com/spotify/heroic/metric/MetricType.java 94.73% <ø> (ø) 6.00 <0.00> (ø)
...main/java/com/spotify/heroic/CoreQueryManager.java 75.21% <ø> (ø) 10.00 <0.00> (ø)
.../main/java/com/spotify/heroic/http/HttpServer.java 16.52% <0.00%> (-0.15%) 3.00 <0.00> (ø)
...potify/heroic/metric/bigtable/BigtableBackend.java 85.81% <ø> (ø) 61.00 <0.00> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3597263...e333355. Read the comment docs.

@sming sming marked this pull request as ready for review January 19, 2021 19:40
@sming sming changed the title DRAFT - DO NOT REVIEW YET - Enforce Mandatory Client ID (Github Issue #704) Enforce Mandatory Client ID (Github Issue #704) Jan 19, 2021
Copy link
Contributor

@malish8632 malish8632 left a comment

Choose a reason for hiding this comment

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

+1

@sming sming merged commit 687ca05 into master Jan 26, 2021
@sming sming deleted the feature/mandatory-client-id branch January 26, 2021 21:07
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.

Enforce Mandatory Client ID
3 participants