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.

Sign up for GitHub

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

Jump to bottom

Initial protobufs for events #6344

Merged

christophermaier merged 66 commits into master from cm/nats-protobuf

May 17, 2019

Contributor

christophermaier commented Mar 26, 2019

Rather than the ad-hoc string serialization used for the prototype,
we'll begin to evolve a Protobuf-based encoding for our events.

The current thinking is that all events will have a "supervisor
metadata" field. Any events that pertain to services will have an
additional "service metadata" field. Any additional information will
be added as fields on the specific concrete event.

For creating events, we'll use a functional interface at the sites
where events are created. These functions then initialize the
protobuf-derived Rust types according to their arguments. When we go
to publish the events, we'll inject the (global) Supervisor metadata,
serialize the whole thing to bytes, and send them to NATS.

Signed-off-by: Christopher Maier cmaier@chef.io

christophermaier self-assigned this

christophermaier requested review from raskchanky, baumanj and gpeers

March 26, 2019 15:37

Contributor

thesentinels commented Mar 26, 2019

Thanks for the pull request! Here is what will happen next:

Your PR will be reviewed by the maintainers
If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

Contributor Author

christophermaier commented Mar 26, 2019

cc: @kmacgugan @afiune @danielsdeleo

gpeers approved these changes

View reviewed changes

gpeers left a comment

looks good to me!

baumanj approved these changes

View reviewed changes

Contributor

baumanj left a comment

Looks good! Just some small thoughts.

components/sup/src/event.rs Show resolved Hide resolved

components/sup/src/event.rs Outdated Show resolved Hide resolved

components/sup/src/event/types.rs Show resolved Hide resolved

danielsdeleo reviewed

View reviewed changes

danielsdeleo left a comment

General format of the protobufs looks good. If proto3 is equally supported in rust then I'd like to switch to it; if not then we can deal.

components/sup/protocols/event.proto Outdated Show resolved Hide resolved

components/sup/protocols/event.proto Outdated Show resolved Hide resolved

raskchanky approved these changes

View reviewed changes

christophermaier force-pushed the cm/nats-protobuf branch from f063765 to 6d1897c Compare

March 27, 2019 21:06

christophermaier requested review from chefsalim and eeyun as code owners

March 28, 2019 15:18

afiune approved these changes

View reviewed changes

afiune left a comment

Looks nice CM. 💯

components/sup/src/event.rs Show resolved Hide resolved

christophermaier force-pushed the cm/nats-protobuf branch 3 times, most recently from 5e810e3 to cbce5f4 Compare

April 11, 2019 17:27

gpeers force-pushed the cm/nats-protobuf branch from 22bc8ea to f3a4af4 Compare

April 17, 2019 17:32

christophermaier force-pushed the cm/nats-protobuf branch from 2eb56d7 to 40f44cc Compare

May 2, 2019 14:41

afiune reviewed

View reviewed changes

components/sup/src/manager/mod.rs Outdated Show resolved Hide resolved

christophermaier force-pushed the cm/nats-protobuf branch from 69ce79a to ad65d61 Compare

May 9, 2019 20:38

baumanj reviewed

View reviewed changes

Contributor

baumanj left a comment

Not finished, but wanted to commit some of my feedback before leaving so you can see it earlier on Monday.

components/common/src/types/mod.rs Outdated Show resolved Hide resolved

components/common/src/types/mod.rs Show resolved Hide resolved

components/common/src/types/mod.rs Outdated Show resolved Hide resolved

components/common/src/types/mod.rs Outdated Show resolved Hide resolved

components/common/src/types/mod.rs Outdated Show resolved Hide resolved

components/sup/protocols/event.proto Show resolved Hide resolved

components/sup/protocols/event.proto Outdated Show resolved Hide resolved

components/sup/protocols/event.proto Outdated Show resolved Hide resolved

components/sup/protocols/event.proto Outdated Show resolved Hide resolved

components/sup/protocols/event.proto Outdated Show resolved Hide resolved

baumanj reviewed

View reviewed changes

Contributor

baumanj left a comment

Still not quite done; this is a lot of code.

components/sup/protocols/event.proto Outdated Show resolved Hide resolved

components/sup/src/cli.rs Show resolved Hide resolved

components/sup/src/error.rs Outdated Show resolved Hide resolved

components/sup/src/error.rs Outdated Show resolved Hide resolved

components/sup/src/event.rs Show resolved Hide resolved

components/sup/src/event/nitox.rs Show resolved Hide resolved

components/sup/src/event/nitox.rs Show resolved Hide resolved

components/sup/src/event/nitox.rs Show resolved Hide resolved

components/sup/src/event/nitox.rs Outdated Show resolved Hide resolved

components/sup/src/event/nitox.rs Outdated Show resolved Hide resolved

baumanj reviewed

View reviewed changes

Contributor

baumanj left a comment

I'm finished with my initial review pass. Let me know when you're happy with everything and I can give it another look for approval.

components/sup/src/event.rs Show resolved Hide resolved

components/sup/src/cli.rs Show resolved Hide resolved

components/sup/src/event/nitox.rs Outdated Show resolved Hide resolved

components/sup/src/event/types.rs Outdated Show resolved Hide resolved

components/sup/src/manager/service/mod.rs Outdated Show resolved Hide resolved

christophermaier force-pushed the cm/nats-protobuf branch 2 times, most recently from 278f33d to 2e11c8f Compare

May 15, 2019 14:02

afiune mentioned this pull request

User real Habitat HealthCheck event message chef/automate#341

Merged

4 tasks

kmacgugan mentioned this pull request

Add studio helper, protos and generated code for habitat messages chef/automate#358

Closed

4 tasks

afiune force-pushed the cm/nats-protobuf branch from 996e14a to e5310c0 Compare

May 15, 2019 22:14

baumanj reviewed

View reviewed changes

Contributor

baumanj left a comment

Two little tweaks; overall looks really great

components/sup/src/manager/service/mod.rs Outdated Show resolved Hide resolved

components/sup/src/event.rs Outdated Show resolved Hide resolved

christophermaier and others added 25 commits

May 17, 2019 17:41


          [PR Feedback] Add extra newline to break things up a bit

c45313c

Signed-off-by: Christopher Maier <cmaier@chef.io>


          [PR Feedback] Use to_string over String::from for better readability

0a5dd12

Signed-off-by: Christopher Maier <cmaier@chef.io>


          [PR Feedback] Remove unnecessary i32 cast

4c66a83

Signed-off-by: Christopher Maier <cmaier@chef.io>


          [PR Feedback] Use named secs field for reasonability

d9d9cd5

Signed-off-by: Christopher Maier <cmaier@chef.io>


          [PR Feedback] Add a more useful error message

1f0392f

Signed-off-by: Christopher Maier <cmaier@chef.io>


          [PR Feedback] Remove TODO about description soft-deprecation

eb64b71

See #6555 instead.

Signed-off-by: Christopher Maier <cmaier@chef.io>


          [PR Feedback] Link error source TODO back to an issue

db421cf

Signed-off-by: Christopher Maier <cmaier@chef.io>


          Implement From<&ArgMatches> for EventStreamConfig

200562d

Signed-off-by: Christopher Maier <cmaier@chef.io>


          [PR Feedback] Remove unused function

ed5afea

Signed-off-by: Christopher Maier <cmaier@chef.io>


          Remove some unnecessary TODOs

883e6ac

Signed-off-by: Christopher Maier <cmaier@chef.io>


          [PR Feedback] Disable some problematic rustfmt effects

c8b03d2

Looks like it's having trouble properly formatting deeply-nested /
chained futures.

Signed-off-by: Christopher Maier <cmaier@chef.io>


          [PR Feedback] Use cfg_attr to consolidate compilation conditions

5e09902

Signed-off-by: Christopher Maier <cmaier@chef.io>


          [PR Feedback] Update -> UpdateConfig

6821e9e

Naming is hard

Signed-off-by: Christopher Maier <cmaier@chef.io>


          Clean up some TODOs

93ab686

Signed-off-by: Christopher Maier <cmaier@chef.io>


          [PR Feedback] Ensure nitox_stream feature can still build

55a4992

Until such time as we decide to remove it, that is.

Signed-off-by: Christopher Maier <cmaier@chef.io>


          [PR Feedback] Renamed 'timestamp' proto field to 'occurred_at'

fd14810

Signed-off-by: Christopher Maier <cmaier@chef.io>


          [PR Feedback] Rename health_check duration field to execution

3c43647

More self-describing this way.

Signed-off-by: Christopher Maier <cmaier@chef.io>


          Backing out cfg_attr changes for now

ab059a6

Looks like `path` attributes are invisible to rustfmt:

- rust-lang/rustfmt#1208
- rust-lang/rustfmt#2407

Ideally, it would be nice to not use the `path` attribute at all, and
we can actually formulate this in a way that doesn't use `path`, but
it seems to involve a lot more repetition. It's not at all clear that
that's a win here, particularly given that we're only going to have
two implementations for a short period of time.

Signed-off-by: Christopher Maier <cmaier@chef.io>


          Add FQDN to EventCore Metadata

87a92a7

From the [`hostname` man page](https://www.systutorials.com/docs/linux/man/1-hostname/):
>The FQDN (Fully Qualified Domain Name) of the system is the
name that the resolver(3) returns for the host name, such as,
**ursula.example.com**.  It is usually the hostname followed by the
DNS domain name (the part  after the first dot).  You can check
the FQDN using `hostname --fqdn` or the domain name using
`dnsdomainname`.

As the man page says it, the FQDN is the combination of the
Hostname and the Domain Name. But technically speaking:
>The FQDN is the name getaddrinfo(3) returns for the
host name returned by gethostname(2). The DNS domain name
is the part after the first dot.

This change is adding two new `core::os::net` functions:
- `lookup_fqdn(hostname)`: returns a vector of fqdn that resolves
the provided hostname.
- `fqdn()`: returns the FQDN (fully qualified domain name) of the
running machine.

These functions are leveraging the [dns-lookup crate](https://docs.rs/dns-lookup/1.0.0/dns_lookup/index.html) that implements
the C function called `getaddrinfo()` that we technically need to get
FQDN of the running machine.

Habitat already have a function that returns the hostname of the
machine[ called `hostname()`](https://github.com/habitat-sh/habitat/blob/master/components/core/src/os/net/windows.rs#L22), we are leveraging that function inside
the new `fqdn()` one.

I have created an issue #6531 to explore the possibility of refactoring the
existing `hostname()` function in favor of `dns_lookup::lookup_host()`

Signed-off-by: Salim Afiune <afiune@chef.io>


          Rename HealthCheck to HealthCheckResult

e865211

Signed-off-by: Christopher Maier <cmaier@chef.io>


          Update dns_lookup deps

862777c

Signed-off-by: Christopher Maier <cmaier@chef.io>


          Implement Site field in EventMetadata message

a793008

The user will be able to set this parameter through an environment
variable or an option in the habitat CLI, just as you will be able to do
with application and environment.

Signed-off-by: Salim Afiune <afiune@chef.io>


          Removes unecessary required(false) options

Signed-off-by: Salim Afiune <afiune@chef.io>


          Fix lint errors

a404a6e

Signed-off-by: Salim Afiune <afiune@chef.io>


          [PR Feedback] Order fields and clean cli flag func

74d7189

Signed-off-by: Salim Afiune <afiune@chef.io>

christophermaier force-pushed the cm/nats-protobuf branch from fc8f02f to 74d7189 Compare

May 17, 2019 21:46

afiune approved these changes

View reviewed changes

afiune left a comment •

edited

Loading

Tested against latest Automate build, everything looks great! Ship it!

christophermaier merged commit b9652b9 into master

christophermaier deleted the cm/nats-protobuf branch

May 17, 2019 23:30

chef-ci added a commit that referenced this pull request


          Update CHANGELOG.md with details from pull request #6344

4527c32

Obvious fix; these changes are the result of automation not creative thinking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

baumanj baumanj approved these changes

danielsdeleo danielsdeleo left review comments

gpeers gpeers approved these changes

afiune afiune approved these changes

raskchanky raskchanky approved these changes

chefsalim Awaiting requested review from chefsalim

eeyun Awaiting requested review from eeyun

Labels

None yet