feat: add rust server and template code SYNC-4447

[taddes]

This feature adds rust_server as one of the glean_parser options. As part of our discussions to integrate Glean into syncstorage-rs, this is to accomplish that goal and allow for other projects to make use of the parser. The logic was modeled after the go_server with a few minor modifications. Template file generates the metrics and events as expected. Tests were also modeled after the go style.

There are a set of 3 tests that generate rust code via cargo and they pass locally, while would require a little more CI config and I wanted to discuss that with you prior, since there may be added overhead with rustup and building via cargo. Tests temporarily disabled.

Pull Request checklist

• Quality: This PR builds and tests run cleanly
  • make test runs without emitting any warnings
  • make lint runs without emitting any errors
• Tests: This PR includes thorough tests or an explanation of why it does not
• Changelog: This PR includes a changelog entry to CHANGELOG.md or an explanation of why it does not need one
  • Any breaking changes to language binding APIs are noted explicitly

[taddes]

Appears when running the added rust tests (just the 3 that temporarily generate Rust projects to compare log output against), the CI fails due to some likely incompatibility with cargo. Think we would need to add it to the docker builds, I can go ahead and give that a shot to see if it resolves.

I also used some newer Python features (match stmt) which I see isn't supported in the earlier builds, so I'll change that to simplify. Note I didn't realize until a few CI failures that the new mypy types weren't supported in older versions of Py. I've been spoiled lately. Reverted those to support the older List and Dict types.

[badboy]

Didn't yet do a full review, but noticed a couple of things early, so submitting them already.

[badboy]

We probably shouldn't just print to stdout?
Not sure what the exact plan was here, but wouldn't we want to integrate with log? Or somehow pass a logger in?

[taddes]

I think in the discussions with @akkomar and @travis79 , we had decided stdout is the way to go for rust server metrics, however I could be mistaken. I will double check with them, but I think this is how it's expected to be picked up.

[akkomar]

How do you normally output logs in Sync @taddes? It would be best to use your standard logging mechanism. We just need to make sure that this log goes to Cloud Logging in a correct format.

[taddes]

We use slog / slog_mozlog_json per SecOps guidelines. I'm going to check with the team and SRE about this.

I'm not sure if in this repo you want the most agnostic approach for output (being stdout) and allowing each service to then modify the output path, however I'm happy to make whatever changes you all see fit for a general purpose rust_server implementation. I saw with the go_server implementation, it just prints to stdout.

[akkomar]

I think it's good to go with the approach recommended by SRE/SecOps in the template. I don't remember why Go template looks this way, maybe that's what they do in their code.

[akkomar]

Thank you for working on this!

I added some suggestions inline. I have tested this with my suggested changes applied in a sandbox project and confirmed that the format is correct.

I still have to review the tests. We could go without adding new test yaml files, but as I mentioned earlier I'm planning to refactor and unify these outputters so this is probably fine.

[taddes]

Excellent @akkomar , thank you for taking the time to review and also to validate in the sandbox. I will be doing the same with my custom project that emulates the data being passed in through Sync. Given each language varies in its serialization logic and whatnot, please let me know if you find anything that warrants a change. I will be sure to do the same.

I've updated the template to be compliant with MozLog (Type and Fields), removing the two timestamp and logger fields from the LogEvelope struct and updated the test files/tests.

We'll confer next week with any other modifications. Now that we're registered with the probe scraper, once Sync is deployed with the Glean logic we'll be able to verify the functionality.

Thanks for the opportunity to contribute to your project and for all the great collaboration!

[akkomar]

Can you add a changelog entry?

Apart from that LGTM.

@badboy if you'd like to look at the sample output I have one in a test project: https://github.com/akkomar/server-telemetry-node-poc/pull/1/files#diff-c694e95fd2dc337599b52405253edd0ffe00fa93e870cb34f36358fe17628c88

[akkomar]

All right, looks like the only unresolved comment we have is about the actual logger:

We use slog / slog_mozlog_json per SecOps guidelines. I'm going to check with the team and SRE about this.

@taddes have you checked with the team/SRE?
I'd rather have this template to use the default recommended logger that you (and others) use, so you can just drop in the generated code into your repo without modifications.

[taddes]

Thanks @akkomar ! Yes, we're just discussing that as slog may be overkill, but regardless we're evaluating the right approach and indeed can add that. I did make a few optimizations to our Sync implementation to make it more "rusty", which is just some minor implementation changes (using Default::default() in a few places, to_owned() instead of to_string(). I'll push those here shortly

[taddes]

Ok @akkomar , in discussion with the team, we agreed stdout is ideal and we are not going to use slog. It would introduce an unnecessary layer that diverges from the desired output. A couple minor changes introduced as specified in my last comment (a few to_owned() calls instead of to_string() and a Default impl for Pinginfo

[akkomar]

LGTM. I'd love @badboy to take a quick look at the generated code (for convenience see here) before merging.

[taddes]

Awesome, thanks @akkomar . I have some thoughts on make some things a bit more "rusty," but we can save that for a subsequent PR. That would be implementing From in a couple places so one can call into() to convert between types, a little restructuring to use more Default::default calls.

[badboy]

generated code looks good to me.
I have a few stylistic nits with respect to the code, but these could also be wrapped into the followup.

[taddes]

Thanks @badboy & @akkomar. I made those doc changes and nits and added some extra documentation, as I did the same on our Sync side. You'll have to re-approve when you have the chance and then feel free to merge, as I am not sure I have write access to the repo. I can then in the near future file another ticket to make some optimizations if we discover some improvements from the Sync side.