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

feat: Allow configuring a dedicated metrics logger #978

Merged
merged 9 commits into from
Oct 4, 2022

Conversation

edgarrmondragon
Copy link
Collaborator

@edgarrmondragon edgarrmondragon commented Sep 17, 2022

Summary

New metrics helpers

The most important difference: the previous implementation only emitted total records at the end of sync, while the new one emits the accumulated record count between regular intervals (60s by default). This allows, among other things, to plot record counts against time for a child and parent stream:

metric

Takes inspiration from singer-python. The new classes actually output time-series data for record count, sync duration and HTTP request duration.

The Point class

This dataclass has a custom __str__() method that renders the object as JSON, that means disabling metrics via log config (e.g. level ERROR) results in not needing to perform the costly object->string conversion and makes testing metrics easier by capturing the raw object from the LogRecord instance.

Logging configuration

Since metrics are just special logs, I think the best way to configure them is via generic log configuration. So, the SDK now supports an SINGER_SDK_LOG_CONFIG env var that points to a logging config file in YAML format (same as meltano).


📚 Documentation preview 📚: https://meltano-sdk--978.org.readthedocs.build/en/978/

@edgarrmondragon edgarrmondragon linked an issue Sep 17, 2022 that may be closed by this pull request
@edgarrmondragon edgarrmondragon force-pushed the 933-feat-emit-metrics-with-a-dedicated-logger branch from 8f74b18 to deee553 Compare September 17, 2022 00:21
@codecov
Copy link

codecov bot commented Sep 17, 2022

Codecov Report

Merging #978 (103a731) into main (3c8e213) will increase coverage by 0.81%.
The diff coverage is 93.50%.

❗ Current head 103a731 differs from pull request most recent head 7be243f. Consider uploading reports for the commit 7be243f to get more accurate results

@@            Coverage Diff             @@
##             main     #978      +/-   ##
==========================================
+ Coverage   82.18%   83.00%   +0.81%     
==========================================
  Files          37       39       +2     
  Lines        3621     3748     +127     
  Branches      625      629       +4     
==========================================
+ Hits         2976     3111     +135     
+ Misses        479      473       -6     
+ Partials      166      164       -2     
Impacted Files Coverage Δ
singer_sdk/streams/core.py 85.30% <86.36%> (+2.16%) ⬆️
singer_sdk/streams/rest.py 84.90% <88.23%> (+1.24%) ⬆️
singer_sdk/metrics.py 96.03% <96.03%> (ø)
singer_sdk/helpers/_resources.py 100.00% <100.00%> (ø)
singer_sdk/plugin_base.py 75.14% <100.00%> (+0.43%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@edgarrmondragon edgarrmondragon force-pushed the 933-feat-emit-metrics-with-a-dedicated-logger branch 4 times, most recently from 9929edb to ae65f65 Compare September 20, 2022 02:55
@edgarrmondragon
Copy link
Collaborator Author

pre-commit.ci autofix

@edgarrmondragon edgarrmondragon changed the title feat: Dedicated metrics output by default feat: Use a default dedicated metrics logger Sep 20, 2022
@edgarrmondragon edgarrmondragon force-pushed the 933-feat-emit-metrics-with-a-dedicated-logger branch 5 times, most recently from 3e5ad46 to d505afb Compare September 23, 2022 19:57
@edgarrmondragon edgarrmondragon force-pushed the 933-feat-emit-metrics-with-a-dedicated-logger branch 4 times, most recently from 26ac1bc to c6a8271 Compare September 28, 2022 06:41
@edgarrmondragon edgarrmondragon force-pushed the 933-feat-emit-metrics-with-a-dedicated-logger branch from c6a8271 to b88d8b5 Compare September 28, 2022 06:43
@edgarrmondragon edgarrmondragon force-pushed the 933-feat-emit-metrics-with-a-dedicated-logger branch from a49bffb to f486643 Compare September 29, 2022 05:28
@edgarrmondragon edgarrmondragon force-pushed the 933-feat-emit-metrics-with-a-dedicated-logger branch from f486643 to 131c498 Compare September 29, 2022 05:46
@edgarrmondragon edgarrmondragon marked this pull request as ready for review September 29, 2022 22:33
@aaronsteers
Copy link
Contributor

Exciting!!

Copy link
Contributor

@cjohnhanson cjohnhanson left a comment

Choose a reason for hiding this comment

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

Looks good! 🚀

@edgarrmondragon edgarrmondragon changed the title feat: Use a default dedicated metrics logger feat: Allow configuring a dedicated metrics logger Sep 30, 2022
@edgarrmondragon edgarrmondragon enabled auto-merge (squash) October 4, 2022 22:03
@edgarrmondragon edgarrmondragon merged commit 6d74490 into main Oct 4, 2022
@edgarrmondragon edgarrmondragon deleted the 933-feat-emit-metrics-with-a-dedicated-logger branch October 4, 2022 22:10
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.

feat: Emit metrics with a dedicated logger
3 participants