Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #475 +/- ##
==========================================
+ Coverage 80.47% 80.67% +0.19%
==========================================
Files 43 43
Lines 4046 4124 +78
==========================================
+ Hits 3256 3327 +71
- Misses 790 797 +7
🚀 New features to boost your workflow:
|
…s into DOG-5996-Implement-metrics
|
Is the metrics module just copied? Do you need to change it? Any reason why you can't just reuse the one already in the stable part of the utils? |
Aren't we moving everything to unstable ? Should I use the stable |
So long as you haven't changed them. You can definitely use Try to think about why we're doing this unstable thing. |
…s into DOG-5996-Implement-metrics
…s into DOG-5996-Implement-metrics
| host: str | ||
| job_name: str | ||
| username: str | None | ||
| password: str | None |
There was a problem hiding this comment.
Won't the lack of defaults here means that these config objects will need an explicit username: null in config?
Could you write a test that actually loads these types from YAML?
There was a problem hiding this comment.
Nice catch, as best practices suggest we should have default value defined here. Fix the default value and also added the test case to if a config variable is missing then it doesn't break.
tests/test_unstable/test_base.py
Outdated
| ], | ||
| "cognite": None, | ||
| "server": None, | ||
| } |
There was a problem hiding this comment.
Are you dumping a struct as YAML then reading it back? This doesn't really verify that None values aren't still required. Instead, you should write out the yaml and just load it directly from a string in the test.
tests/test_unstable/test_base.py
Outdated
| clear_after: null | ||
| username: null | ||
| cognite: null | ||
| server: null |
There was a problem hiding this comment.
The thing I wanted to verify was that this works without explicitly setting x: null, so leave those out. We generally don't encourage explicitly setting things to null in yaml configs.
There was a problem hiding this comment.
Here, password is not set explicitly to null, so this is working (I want to verify if explicitly setting to null is also working i.e. username).
There was a problem hiding this comment.
Then add another test where you don't set them to null explicitly. Setting them to null is the exception, not the rule, and I would prefer if tests verified the intended usage as well as any corner cases.
There was a problem hiding this comment.
The thing I wanted to verify was that this works without explicitly setting
x: null, so leave those out. We generally don't encourage explicitly setting things to null in yaml configs.
Made the changes, I got your point why we do require to set the defaults and not passing null explicitly.
Hmnt39
added
the
waiting-for-risk-review
| if push_gateway.clear_after is not None: | ||
| self.clear_on_stop[prometheus_pusher] = push_gateway.clear_after.seconds | ||
|
|
||
| if self.metrics_config.cognite: |
arnels
added
waiting-for-team
4 participants
This PR introduces the metrics and pusher manager for Cognite extractors, enabling flexible metrics collection, singleton enforcement, and reliable pushing to Prometheus and Cognite Data Fusion (CDF).