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

Rename debug to inspect, add implementation guide, other improvements #310

Merged
merged 6 commits into from
Dec 1, 2021

Conversation

m-mohr
Copy link
Member

@m-mohr m-mohr commented Dec 1, 2021

As this process is still very debated, it probably helps to demystify some things.

  • Renamed to log. debug was too closely bound to the log levels and as such confusing. log also is more closely bound to the endpoint names and naming in clients.
  • The log level error does not need to stop execution.
  • Added proposals for logging several data types to the implementation guide.
  • Mention that using this in callbacks is potentially a bad idea.

@soxofaan
Copy link
Member

soxofaan commented Dec 1, 2021

don't we already have log for logarithm?

@soxofaan
Copy link
Member

soxofaan commented Dec 1, 2021

I would also propose to rename the code argument to something like tag or label, which is clearer by itself.
code (and identifier as used in its description) make it sound like some unique id.

Also something to consider is to give this tag argument another default than empty string (e.g. "user") so that "user" debug logs can be found easily by default. Then this argument also can be moved to last position, because it feels less important than level or message to me.

@m-mohr
Copy link
Member Author

m-mohr commented Dec 1, 2021

don't we already have log for logarithm?

Yikes, very good point! Proposals for another name?

@m-mohr
Copy link
Member Author

m-mohr commented Dec 1, 2021

I would also propose to rename the code argument to something like tag or label, which is clearer by itself. code (and identifier as used in its description) make it sound like some unique id.

The parameter names here correspond to the API field names for logs, so I think that's a good reason to make clear which parameter corresponds to which field. We can fine-tune the description though.

Also something to consider is to give this tag argument another default than empty string (e.g. "user") so that "user" debug logs can be found easily by default.

What would be a (static?) alternative? Maybe record, log_data or inspect?

Then this argument also can be moved to last position, because it feels less important than level or message to me.

I don't think it's as important as message, but probably less important than level. Although not sure how often people actually change the level?

@soxofaan
Copy link
Member

soxofaan commented Dec 1, 2021

The parameter names here correspond to the API field names for logs

Ok fair enough. The description should indeed make it sound less than a unique id.

give this tag argument another default than empty string (e.g. "user") so that "user" debug logs can be found easily by default.

What would be a (static?) alternative? Maybe log_data or inspect?

I think we're misunderstanding each other here. I meant tot give the argument code the static string "user" as default. The idea is that the majority of logs will be generated by the back-end software stack and that user generated logs will be a lot more rare. By giving them a default label, it will be easier to filter them out. Clients could even highlight "user" logs or something like that.

I don't think it's as important as message, but probably less important than level. Although not sure how often people actually change the level?

I'm also not sure I understand fully your reply.
In any case I'm also questioning if changing the level is something real users will do. openEO users have basically not the control structures to do the kind of helpful logging you do in "normal" development. They can basically just "log" the current state of a datacube or some static string (which by itself is a questionable feature), but why would you do that with level "warning" or "error"?

Yikes, very good point! Proposals for another name?

Not sure, both "log" and "debug" are already fairly overloaded in meaning.
If the message argument can be dropped, maybe data_info or indeed "inspect"

@m-mohr
Copy link
Member Author

m-mohr commented Dec 1, 2021

The description should indeed make it sound less than a unique id.

Done.

I think we're misunderstanding each other here. I meant tot give the argument code the static string "user" as default.

Uh, yes, that was a misunderstanding. That's a good idea. User is fine for me, added.

I'm also questioning if changing the level is something real users will do.

I could see this being useful, e.g. if you want to raise the log level in jobs to not get so much noise but still want to see your log output. It's optional anyway and available in the log entries the API exposes, so have not a big issue in having it.

maybe data_info or indeed "inspect"

Another alternative that came to mind is "record", but changed to "inspect" for now to at least resolve the conflict.

If the message argument can be dropped

Actually, I just checked the API documentation and there the message is required. I guess that was the reason for defaulting to an empty string. Well, ideally we would remove the "message" requirement in the API as it's not very useful to circumvent it with an empty string anyway.

proposals/inspect.json Outdated Show resolved Hide resolved
@soxofaan
Copy link
Member

soxofaan commented Dec 1, 2021

If the message argument can be dropped

Actually, I just checked the API documentation and there the message is required. I guess that was the reason for defaulting to an empty string. Well, ideally we would remove the "message" requirement in the API as it's not very useful to circumvent it with an empty string anyway.

Well, the message field is indeed (obviously) required for the logs you retrieve from endpoints like /jobs/{job_id}/logs, but this inspect process is to write logs where the message is a summary of data, so you don't need an additional message argument in inspect. Or how would a back-end have to combine the data summary with the message argument in a single message field in the logs?

@m-mohr
Copy link
Member Author

m-mohr commented Dec 1, 2021

There's a message and a data property in the logs. You that is a 1:1 mapping between the process and the API, with the exception that if the data is infeasible to dump completely, a summary should be provided as proposed in the implementation guide. So the data doesn't need to be squeezed into a message string, of course. Clients can nicely render machine-readable JSON from the data property, e.g. the dimension overview from vue components if dimensions are included in the data property for a raster cube. The message can contain additional details from the user (or be empty).

@m-mohr m-mohr changed the title Rename debug to log, add implementation guide, other improvements Rename debug to inspect, add implementation guide, other improvements Dec 1, 2021
@m-mohr m-mohr merged commit 1a954f6 into draft Dec 1, 2021
@m-mohr m-mohr deleted the debug-log branch December 1, 2021 17:03
@soxofaan
Copy link
Member

soxofaan commented Dec 1, 2021

ah ok, I wasn't aware of (or at least didn't remember) this data property in the logs. That's actually a good reminder for when we improve the logging support in VITO backend, which (like other back-ends I assume) is text message based, so we have to make sure that the data structure can be parsed properly from it

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