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

[plugins/prometheus] Fix registry metrics duplication #2217

Merged
merged 10 commits into from
May 7, 2024

Conversation

EmrysMyrddin
Copy link
Collaborator

@EmrysMyrddin EmrysMyrddin commented Apr 26, 2024

Description

Is this plugin is initialized multiple times with the same registry (which can happen in the context of Mesh when polling is enabled for example).

This PR aims to fix this.

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as
    expected)

Further comments

Current solution rely on caching Histograms with a Map to reuse them instead of creating them each time.

But this has a drawback: if the configuration of those Histograms changes, this changement will be silently ignored and the old configuration will be kept in use.

Copy link

changeset-bot bot commented Apr 26, 2024

🦋 Changeset detected

Latest commit: 5841b48

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@envelop/prometheus Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@EmrysMyrddin EmrysMyrddin changed the title Fix registry metrics duplication [plugins/prometheus] Fix registry metrics duplication Apr 26, 2024
Copy link
Contributor

github-actions bot commented Apr 26, 2024

💻 Website Preview

The latest changes are available as preview in: https://587986fa.envelop.pages.dev

@EmrysMyrddin EmrysMyrddin force-pushed the prometheus/fix-duplicated-metrics branch 2 times, most recently from 54ead08 to 78d967c Compare April 26, 2024 14:59
Copy link
Contributor

github-actions bot commented Apr 26, 2024

🚀 Snapshot Release (alpha)

The latest changes of this PR are available as alpha on npm (based on the declared changesets):

Package Version Info
@envelop/opentelemetry 6.3.1-alpha-20240507125432-5841b485 npm ↗︎ unpkg ↗︎
@envelop/prometheus 10.0.0-alpha-20240507125432-5841b485 npm ↗︎ unpkg ↗︎

@EmrysMyrddin EmrysMyrddin force-pushed the prometheus/fix-duplicated-metrics branch from 78d967c to 7849bf0 Compare April 26, 2024 17:17
@theguild-bot
Copy link
Collaborator

theguild-bot commented Apr 26, 2024

✅ Benchmark Results

     ✓ no_errors
     ✓ expected_result

     checks.............................................: 100.00% ✓ 880594      ✗ 0     
     ✓ { mode:envelop-cache-and-no-internal-tracing }...: 100.00% ✓ 214734      ✗ 0     
     ✓ { mode:envelop-cache-jit }.......................: 100.00% ✓ 338712      ✗ 0     
     ✓ { mode:envelop-just-cache }......................: 100.00% ✓ 211822      ✗ 0     
     ✓ { mode:graphql-js }..............................: 100.00% ✓ 115326      ✗ 0     
     data_received......................................: 3.4 GB  28 MB/s
     data_sent..........................................: 192 MB  1.6 MB/s
     envelop_init.......................................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     ✓ { mode:envelop-cache-jit }.......................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     ✓ { mode:envelop-just-cache }......................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     ✓ { mode:graphql-js }..............................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     envelop_total......................................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     ✓ { mode:envelop-cache-jit }.......................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     ✓ { mode:envelop-just-cache }......................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     ✓ { mode:graphql-js }..............................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     event_loop_lag.....................................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     ✓ { mode:envelop-cache-and-no-internal-tracing }...: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     ✓ { mode:envelop-cache-jit }.......................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     ✓ { mode:envelop-just-cache }......................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     ✓ { mode:graphql-js }..............................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     graphql_context....................................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     ✓ { mode:envelop-cache-jit }.......................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     ✓ { mode:envelop-just-cache }......................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     ✓ { mode:graphql-js }..............................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     graphql_execute....................................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     ✓ { mode:envelop-cache-jit }.......................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     ✓ { mode:envelop-just-cache }......................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     ✓ { mode:graphql-js }..............................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     graphql_parse......................................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     ✓ { mode:envelop-cache-jit }.......................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     ✓ { mode:envelop-just-cache }......................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     ✓ { mode:graphql-js }..............................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     graphql_validate...................................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     ✓ { mode:envelop-cache-jit }.......................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     ✓ { mode:envelop-just-cache }......................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     ✓ { mode:graphql-js }..............................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     http_req_blocked...................................: avg=1.83µs  min=722ns    med=1.66µs  max=8.22ms  p(90)=2.22µs  p(95)=2.38µs 
     http_req_connecting................................: avg=21ns    min=0s       med=0s      max=1.45ms  p(90)=0s      p(95)=0s     
     http_req_duration..................................: avg=2.45ms  min=150.6µs  med=2.12ms  max=81.81ms p(90)=4.46ms  p(95)=4.92ms 
       { expected_response:true }.......................: avg=2.45ms  min=150.6µs  med=2.12ms  max=81.81ms p(90)=4.46ms  p(95)=4.92ms 
     ✓ { mode:envelop-cache-and-no-internal-tracing }...: avg=2.52ms  min=327.02µs med=2.21ms  max=17.66ms p(90)=4.4ms   p(95)=4.79ms 
     ✓ { mode:envelop-cache-jit }.......................: avg=1.48ms  min=150.6µs  med=1.19ms  max=15.58ms p(90)=2.43ms  p(95)=2.59ms 
     ✓ { mode:envelop-just-cache }......................: avg=2.55ms  min=370.01µs med=2.23ms  max=34.31ms p(90)=4.43ms  p(95)=4.83ms 
     ✓ { mode:graphql-js }..............................: avg=4.95ms  min=698.55µs med=4.17ms  max=81.81ms p(90)=8.3ms   p(95)=8.92ms 
     http_req_failed....................................: 0.00%   ✓ 0           ✗ 440297
     http_req_receiving.................................: avg=30.47µs min=12.11µs  med=27.41µs max=12.51ms p(90)=40.64µs p(95)=45.19µs
     http_req_sending...................................: avg=11.62µs min=4.47µs   med=9.91µs  max=9.17ms  p(90)=13.77µs p(95)=17.4µs 
     http_req_tls_handshaking...........................: avg=0s      min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     http_req_waiting...................................: avg=2.4ms   min=124.13µs med=2.07ms  max=81.76ms p(90)=4.42ms  p(95)=4.87ms 
     http_reqs..........................................: 440297  3669.080993/s
     iteration_duration.................................: avg=2.71ms  min=358.18µs med=2.38ms  max=82.33ms p(90)=4.72ms  p(95)=5.21ms 
     iterations.........................................: 440297  3669.080993/s
     vus................................................: 10      min=10        max=10  
     vus_max............................................: 20      min=20        max=20  

@EmrysMyrddin EmrysMyrddin force-pushed the prometheus/fix-duplicated-metrics branch from 7849bf0 to 27bbd4d Compare April 28, 2024 12:19
@EmrysMyrddin EmrysMyrddin force-pushed the prometheus/fix-duplicated-metrics branch 3 times, most recently from e76039a to 0c0064c Compare April 29, 2024 14:17
@EmrysMyrddin EmrysMyrddin force-pushed the prometheus/fix-duplicated-metrics branch from 0c0064c to bb3fd9f Compare April 29, 2024 14:21
@dotansimha
Copy link
Collaborator

this changement will be silently ignored and the old configuration will be kept in use.
By "silent", you mean without any warning? should we at least warn or print something to the log? how silent it is?

@EmrysMyrddin
Copy link
Collaborator Author

@dotansimha We don't actually have any means to know if the configuration have changed or not. Even if we keep track of the previous configuration somehow and check fo the deep equality with the new config, it will most likely be always different, because of the fillLabels being a function, which will always be uniq even if it has the exact same implementation.

We could add a warning if we detect that a metrics was already registered with this name, but then will have this warning every time we create a new Mesh instance when polling is enable.

Do you think I should add a warning with an option to disable it explicitly ?

@EmrysMyrddin EmrysMyrddin merged commit 7ac1d3c into main May 7, 2024
16 checks passed
@EmrysMyrddin EmrysMyrddin deleted the prometheus/fix-duplicated-metrics branch May 7, 2024 14:35
This was referenced May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants