Skip to content

Change the completion message to something that looks more like success#432

Closed
ragnar-cognite wants to merge 1 commit intomasterfrom
better-completion-message
Closed

Change the completion message to something that looks more like success#432
ragnar-cognite wants to merge 1 commit intomasterfrom
better-completion-message

Conversation

@ragnar-cognite
Copy link
Copy Markdown

  • It shows up in CDF GUI

@ragnar-cognite ragnar-cognite requested a review from a team as a code owner February 18, 2025 15:13
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.22%. Comparing base (e8b1783) to head (fdbd787).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #432      +/-   ##
==========================================
+ Coverage   75.19%   75.22%   +0.02%     
==========================================
  Files          42       42              
  Lines        3495     3495              
==========================================
+ Hits         2628     2629       +1     
+ Misses        867      866       -1     
Files with missing lines Coverage Δ
cognite/extractorutils/base.py 49.19% <ø> (ø)

... and 1 file with indirect coverage changes

@mathialo
Copy link
Copy Markdown
Contributor

Thanks for your PR! I'd prefer the approach from this (it seems abandoned) PR: #276, as it both doesn't automatically change the behavior of every extractor, and it allows more flexibility for users.

I guess we have two options here, we can close this and pick up that PR from the other user, or we can adapt this on and close the other. I looked at the other PR, and it seems master has drifted too far, so I propose just updating and merging that.

@ragnar-cognite
Copy link
Copy Markdown
Author

I tried to do a single line change just to see if I got the process right before I ventured into doing actual meaningful changes :-).

What I really want is a dynamic message based on the outcome of the extractor run. So I guess something similar to the other PR you point to, except calling a function that will return a string so I can log stats as they look when we complete. I would pursue that as a separate PR.

But back to my single liner here: Are there really anyone out there that would prefer to get a success message saying something shut down? If I opened the GUI and saw that as my only message, I would think something went wrong and it died on me. I would also think part of the reason for using a framework like the extractor utils is to get a free ride for general improvements. Think of all the people out there we could make happy because CDF started telling them something went well, opposed to shutting down? :-).

The only thing I can think of breaking from such a chance is if somebody chain processing steps after each other and scan CDF logs looking for a shut down message. But if you rely on such brittle logic, you have a lot of bigger problems in the world.

But I do realize I am new, and may miss something here. So please educate me and tell me where I went wrong!

@mathialo
Copy link
Copy Markdown
Contributor

Are there really anyone out there that would prefer to get a success message saying something shut down?

This isn't as much about what message someone would prefer to see in the front end, but what other systems are depending on. Extraction pipelines is a service that has been used in many different ways by many different users, and several users have built other systems - such as their own monitoring and alerting applications that reads data from extraction pipelines.

Changing what this value is for every extractor, which is what this PR would do, risk breaking any deployment where someone is depending on the content of messages. We're generally very weary of breaking any behavior in extractors outside of major releases.

@ragnar-cognite
Copy link
Copy Markdown
Author

OK, got it. Parking this. Might raise built in support for overriding completion messages again in the future.

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.

2 participants