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

Error categorization guidelines & design decision splitup #2682

Conversation

Projects
None yet
3 participants
@Piankero
Copy link
Contributor

commented May 9, 2019

Basics

Check relevant points but please do not remove entries.
Do not describe the purpose of this PR in the PR description but:

  • Short descriptions should be in the release notes (added as entry in
    doc/news/_preparation_next_release.md which contains _(my name)_)
    Please always add something to the the release notes.
  • Longer descriptions should be in documentation or in design decisions.
  • Describe details of how you changed the code in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, should be in the commit messages.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

  • I added unit tests
  • I ran all tests locally and everything went fine
  • affected documentation is fixed
  • I added code comments, logging, and assertions (see doc/CODING.md)
  • meta data is updated (e.g. README.md of plugins and doc/METADATA.md)

Review

Remove the line below and add the "work in progress" label if you do
not want the PR to be reviewed:

@markus2330 please review my pull request

Merging

Please add the "ready to merge" label when the build server and you say
that everything is ready to be merged.

@Piankero Piankero requested a review from markus2330 May 9, 2019

@Piankero Piankero referenced this pull request May 9, 2019

Closed

New error codes #2589

3 of 9 tasks complete

@Piankero Piankero force-pushed the Piankero:design-decision-split-and-error-categorization branch from 9554d54 to 91ae4dc May 14, 2019

Michael Zronek added some commits May 14, 2019

@markus2330
Copy link
Contributor

left a comment

Thank you, it is much better now!

who are iterating (or continually incrementing/ decrementing) through a space and can easily detect when they are
done by looking for this error.

## Related

This comment has been minimized.

Copy link
@markus2330

markus2330 May 14, 2019

Contributor

Please stick to the doc/decisions/template.md

This comment has been minimized.

Copy link
@Piankero

Piankero May 14, 2019

Author Contributor

This is no "decision" in a narrow sense but a guideline and also therefore I purposefully deleted "Decision"

This comment has been minimized.

Copy link
@Piankero

Piankero May 14, 2019

Author Contributor

Also it is under a different path

This comment has been minimized.

Copy link
@markus2330

markus2330 May 14, 2019

Contributor

Ok! "Related" by itself is, however, unclear.

@@ -0,0 +1,167 @@
# Error Categorization

In this document we start to explain how to categorize errors.

This comment has been minimized.

Copy link
@markus2330

markus2330 May 14, 2019

Contributor

I hope we also finish to explain (and not only start).

# Error Categorization

In this document we start to explain how to categorize errors.
Many errors are unique and can be difficult to categorize so this

This comment has been minimized.

Copy link
@markus2330

markus2330 May 14, 2019

Contributor

Of course every error is unique but this does not mean that the categorization cannot still be useful.

differently on programmatically**. One could argue that an application could always react differently
based on the error (eg. give yourself write or read permission) but such detailed granularity
is rarely needed and would come with a lot of maintenance effort. In the previous versions of
Elektra we were facing many duplicated errors because too many existed

This comment has been minimized.

Copy link
@markus2330

markus2330 May 14, 2019

Contributor

Please rewrite. Make short and precise sentences.

the application just knows that it simply cannot access the desired resource and tells
the user to grant it.

Categories are leaf based, so you cannot put an error

This comment has been minimized.

Copy link
@markus2330

markus2330 May 14, 2019

Contributor

What is "leaf-based". You mean that only leaf-categories can be used?

Why having this restriction? This restriction forces us to introduce otherwise not needed categories such as "General Resource".

And the even bigger problem is that it would disallow to add leaves to an already in-use category.

This comment has been minimized.

Copy link
@Piankero

Piankero May 14, 2019

Author Contributor

Why having this restriction? This restriction forces us to introduce otherwise not needed categories such as "General Resource".

To avoid that users put it in very general categories

And the even bigger problem is that it would disallow to add leaves to an already in-use category.

Why? Extending an existing non-leaf category is easy. Just splitting requires more attention. This makes sense in my opinion because if someone thinks of cases which deserve special handling it deserves its own category and should be clearly distinguishable. New developers might just put it in a non-leaf category despite it deserves this special handling

This comment has been minimized.

Copy link
@markus2330

markus2330 May 14, 2019

Contributor

We could give some categories numbers (like "Resource") and others not (like "Validation). Then we could decide on case-per-case if the category is "too general to be used".

Why?

Let us assume that we need a further category below "Timeout", e.g. "Resolvable By Automatic-Retry". Then we would only move the errors that belong there to this category. Others we would keep directly in "Timeout" (these errors do not need to be touched). I think you understand that going through all errors is not something everyone wants to do.

This comment has been minimized.

Copy link
@Piankero

Piankero May 15, 2019

Author Contributor

We could give some categories numbers (like "Resource") and others not (like "Validation). Then we could decide on case-per-case if the category is "too general to be used".

We could do that but then developers will use too broad error categories instead of good specific ones. It is really against the mindset of the decision. The branches are
only used so that api users can react to whole categories of errors instead of specific errors. It also increases your effort to tell devs in every PR if they might want to use a better specific code.

Others we would keep directly in "Timeout" (these errors do not need to be touched). I think you understand that going through all errors is not something everyone wants to do.

These errors still do not need to be touched because one could rename "Timeout" to another category and change the error code. It is a matter of seconds to do that. (eg. Timeout before: C03000 ... now General Timout: C03100 & "Resolved By Automatic-Retry" C03200)


### Timeout ("C03000")

`Timeout Errors` are easy to categorize as the reaction simply suggests to wait for a short period and retry again.

This comment has been minimized.

Copy link
@markus2330

markus2330 May 14, 2019

Contributor

All errors are easy to categorize when you know the category. (Please rewrite according to structure above)

#### Syntactic ("C04100")

`Syntactic Errors` are errors which tell users or applications that the current format is not valid.
Examples are wrong date formats or missing closing brackets `]` inside of a regular expression. Also path related errors

This comment has been minimized.

Copy link
@markus2330

markus2330 May 14, 2019

Contributor

If the regular expression is checked or if the regular expression is used?

like missing slashes come into this category. Also parsing errors fall under syntactic errors such as
unexpected encounters like missing line endings, illegal characters or wrong encodings. Also transformation
and conversion errors are to be categorizes here because the format of the given input does not allow such
actions. Users should try a different format and retry pushing it to Elektra.

This comment has been minimized.

Copy link
@markus2330

markus2330 May 14, 2019

Contributor
Suggested change
actions. Users should try a different format and retry pushing it to Elektra.
actions. Users should try a different value and retry setting it with Elektra.

#### Semantic ("C04200")

`Semantic Errors` are a bit more tricky to solve programmatically. Examples are references to

This comment has been minimized.

Copy link
@markus2330

markus2330 May 14, 2019

Contributor

Isn't semantic validation now the same as resource errors?

You need a more careful distinction here.

I would say:

  • C01100: an essential resource, needed for Elektra to operate is missing (e.g. cannot write to config file)
  • C04200: a resource, which is marked essential by the configuration specification is missing (e.g. log file required by application not there)

The handling, however, is usually the same: provide the resource. But if the admin does not want to provide the resource, the handling is different:

  • C01100: remount
  • C04200: change spec of application OR retry with different value
### Out of Range ("C05000")

`Out of Range Errors` are operations which attempt to be past a valid range. Examples are trying to set
an Elektra Array entry to `-1` or trying to set a port outside of a valid range. There is quite some overlap

This comment has been minimized.

Copy link
@markus2330

markus2330 May 14, 2019

Contributor

"There is quite some overlap" then we should not have the error. Try to properly distinguish.

If it is only about Elektra's arrays, Validation/Out of Range would be better?

Or we create Validation/Structure for problems in the structure of the KeySet?

@sanssecours Can you maybe help out?

This comment has been minimized.

Copy link
@sanssecours

sanssecours May 14, 2019

Member

Can you maybe help out?

Nope, sorry. I do not know anything about error categorization and I am also currently not interested in learning more about the topic.

This comment has been minimized.

Copy link
@markus2330

markus2330 May 14, 2019

Contributor

Then I think we should remove this category.

Validation/Structure might be useful to pursuit, though.

This comment has been minimized.

Copy link
@Piankero

Piankero May 14, 2019

Author Contributor

I will merge it back to validation. It was initially here because some errors suggested by sanssecours were difficult to get right at first. This category would be a very clear and intuitive category despite some overlap.

Such overlaps are btw not uncommon in other error frameworks too such as gRPC

@Piankero Piankero requested a review from markus2330 May 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.