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(metrics): add option to specify dataset. #271

Conversation

asahaf
Copy link
Contributor

@asahaf asahaf commented Dec 11, 2023

Add option to specify data set under which metrics timeseries will be created under when pushing metrics to cognite.

Add option to specify data set under which metrics timeseries will be
created under when pushing metrics to cognite.
@asahaf asahaf requested a review from a team as a code owner December 11, 2023 11:07
Copy link
Collaborator

@mathialo mathialo left a comment

Choose a reason for hiding this comment

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

Hi! And thanks for your contribution.

I think the linting will fail when it's run, but until then I think there are some style mistakes here. Run pre-commit run --all locally to fix them.

Also please update the changelog.

@asahaf asahaf requested a review from mathialo December 12, 2023 08:44
@asahaf
Copy link
Contributor Author

asahaf commented Dec 12, 2023

I've fixed the style and updated change log. However, pre-commit reports errors that are not related to the updated files/classes

@mathialo
Copy link
Collaborator

The failing CI run is from the tests, and it's related to the updated files. This PR introduces a circular dependency which makes the tests (or anything else) incapable of importing anything from either module:

  • cognite.extractorutils.metrics depends on cognite.extractorutils.configtools.elements (imports EitherIdConfig)
  • cognite.extractorutils.configtools.elements depends on cognite.extractorutils.metrics (it imports the pusher classes, of which CognitePusher uses EitherIdConfig)

This would cause an infinite recursion if the import system didn't catch it.

The metrics module can not depend on the configtools.elements module, neither should it. You should rewrite the change to the CognitePusher to take in an EitherId object instead of an EitherIdConfig, and in the start_pushers method of MetricsConfig, you should initialize the EitherId from the EitherIdConifg.

Hope that helps!

@asahaf
Copy link
Contributor Author

asahaf commented Dec 12, 2023

@mathialo replaced EitherIdConfig with EitherId. pre-commit passing now

@mathialo
Copy link
Collaborator

Great!

The remaining failures seem to be related to some credentials needed for integration tests, which are not available to PRs from external repos. I'll merge this into an intermediate branch, and add a new PR from that branch.

@mathialo mathialo changed the base branch from master to external-pr-intermediate December 12, 2023 12:34
@mathialo mathialo merged commit 6c43847 into cognitedata:external-pr-intermediate Dec 12, 2023
1 of 5 checks passed
@asahaf
Copy link
Contributor Author

asahaf commented Dec 12, 2023

Thanks @mathialo

@mathialo
Copy link
Collaborator

When this merges, the feature should be released #272

mathialo added a commit that referenced this pull request Dec 13, 2023
Add option to specify data set under which metrics timeseries will be
created under when pushing metrics to cognite.

Co-authored-by: Ahmed AlSahaf <[email protected]>
Co-authored-by: Babatunde Aromire <[email protected]>
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.

3 participants