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

Add event categories #297

Merged

Conversation

m-linner-ericsson
Copy link
Member

@m-linner-ericsson m-linner-ericsson commented Mar 16, 2022

Applicable Issues

Solves AP from TC see action list from 10 of March

Description of the Change

Groups events into categories that can be used reasoning about them or to color them when drawing them in our examples

Alternate Designs

Selection of colors and groups should be discussed in this PR

Benefits

We have defined colors for our examples and it makes it easier for people reasoning about them.

Possible Drawbacks

None that I can think of

Sign-off

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

Signed-off-by: Mattias Linnér mattias.linner@ericsson.com

@m-linner-ericsson m-linner-ericsson self-assigned this Mar 23, 2022
@e-backmark-ericsson
Copy link
Member

I believe it would be good to give each of these categories a dedicated color as well, to be used in our event graphs all over the protocol repo. For example here: https://github.com/eiffel-community/eiffel/blob/master/usage-examples/pipeline-monitoring.md#event-graph

@m-linner-ericsson
Copy link
Member Author

m-linner-ericsson commented Jul 27, 2022

@e-backmark-ericsson Any opinions on the colors suggested?

@m-linner-ericsson m-linner-ericsson marked this pull request as ready for review July 29, 2022 15:01
@m-linner-ericsson m-linner-ericsson requested a review from a team as a code owner July 29, 2022 15:01
@m-linner-ericsson m-linner-ericsson modified the milestone: Edition Arica Aug 3, 2022
@e-backmark-ericsson
Copy link
Member

Do we think that we should recommend/propose these categories to be used as the event "family" in the routing key syntax described here: https://eiffel-community.github.io/eiffel-sepia/rabbitmq-message-broker.html ? If so, I believe we should make sure this categorization is as stable as possible over time and thereby spend some conscious thoughts on this PR before accepting it, as some routing/federation implementations might depend on those families. If not, it could be ok to let this PR through without too deep discussions.

Copy link
Member

@magnusbaeck magnusbaeck left a comment

Choose a reason for hiding this comment

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

Emil put it well. If we just plan on using this in documentation then it's fine as it is, otherwise we need to be quite careful.

eiffel-syntax-and-usage/event-categories.md Outdated Show resolved Hide resolved
eiffel-syntax-and-usage/event-categories.md Outdated Show resolved Hide resolved
tox.ini Outdated
@@ -28,7 +28,7 @@ basepython = python3

[testenv:black]
deps =
black==21.12b0
black==22.3.0
Copy link
Member

Choose a reason for hiding this comment

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

I've filed #307 for the Black error. Any PR that includes the patch above should reference that issue but ideally I think we should have a separate PR for this bug.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed with 018db12

@m-linner-ericsson
Copy link
Member Author

Do we think that we should recommend/propose these categories to be used as the event "family" in the routing key syntax described here: https://eiffel-community.github.io/eiffel-sepia/rabbitmq-message-broker.html ? If so, I believe we should make sure this categorization is as stable as possible over time and thereby spend some conscious thoughts on this PR before accepting it, as some routing/federation implementations might depend on those families. If not, it could be ok to let this PR through without too deep discussions.

Agree with you both @e-backmark-ericsson and @magnusbaeck but do you have any opinions on which path to take? Is there any value of not using it for the routing key?

@magnusbaeck
Copy link
Member

If we have a well-defined categorization I think it would be strange to not use it in the routing key now that we have a family field there. I'm not a fan of that field, but since it's there we might as well make the best of it. Maybe the proposed categorization is as good as it's going to get?

Copy link
Member

@magnusbaeck magnusbaeck left a comment

Choose a reason for hiding this comment

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

Argh, my comment two days ago was a PR-level comment and not a Review™ so it didn't include my draft inline comment.

tox.ini Outdated
@@ -28,7 +28,8 @@ basepython = python3

[testenv:black]
deps =
black==22.6.0
black==22.6.0

Copy link
Member

Choose a reason for hiding this comment

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

No big deal, but you're adding a blank line and changing the indentation of line 31.

t-persson
t-persson previously approved these changes Sep 29, 2022
Copy link
Member

@e-backmark-ericsson e-backmark-ericsson left a comment

Choose a reason for hiding this comment

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

I'm ok with approving this as is now, with the addition that it should be brought up on the Eiffel Summit in a couple of weeks. There we should collect any thoughts from that audience on if further adjustments need to be done to it before recommending it to be used as the family field in the routing key through the Sepia architecture docs.

@magnusbaeck
Copy link
Member

Let's merge this, but please note that there's a conflict to resolve first. Also, perhaps align the tox.ini to what's currently on master while you're at it?

@m-linner-ericsson
Copy link
Member Author

@e-backmark-ericsson @t-persson @magnusbaeck need your approvals again due to the updates I made

@m-linner-ericsson m-linner-ericsson merged commit 7f2f525 into eiffel-community:master Oct 11, 2022
@m-linner-ericsson m-linner-ericsson deleted the add_event_categories branch October 11, 2022 19:16
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.

None yet

4 participants