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

Go server support #645

Merged
merged 1 commit into from
Jan 17, 2024
Merged

Conversation

dmueller
Copy link
Contributor

@dmueller dmueller commented Dec 19, 2023

Problem

We're getting a Go service up and running and want to be able to get some business metrics that will help inform our decisions and awareness of how the ads funnel is performing. Some of these metrics will correlate with client telemetry, so we want to be able to collect them and analyze them in BQ for convenience (to avoid having to compare data from service monitoring dashboard and then have difficulty correlating them, or worry about having different retention durations between data in different systems)

Solution

Following the patterns of ruby_server and js_server, this adds support for go_server. It only supports the "events" ping and doesn't support custom pings -- this is a difference between ruby_server and js_server, but it was an intentional decision to leave it out from ruby_server and we want that to be the pattern going forward for server implementations.

The go_server implementation supports all three currently allowed types in the extra_keys field for event metrics, and supports string and quantity metrics as well. We expect numeric data to be common (e.g. how many ads were returned to the client), so that is another difference between go_server and the other two existing ones.

The generated code hasn't yet been deployed in GCP to confirm the flow in its entirety, so there may be some follow up adjustments with that step.

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

@dmueller dmueller changed the title Go server support Go server support (WIP) Dec 19, 2023
@dmueller dmueller marked this pull request as ready for review December 21, 2023 00:05
@dmueller dmueller changed the title Go server support (WIP) Go server support Dec 21, 2023
Copy link
Collaborator

@akkomar akkomar left a comment

Choose a reason for hiding this comment

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

r+wc

LGTM, I have successfully tested this end to end in a staging project.

glean_parser/go_server.py Outdated Show resolved Hide resolved
glean_parser/go_server.py Show resolved Hide resolved
@akkomar akkomar requested a review from badboy January 9, 2024 11:09
Copy link
Member

@badboy badboy left a comment

Choose a reason for hiding this comment

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

one thing blocking this, once fixed this should be good to go.

Comment on lines +4 to +6
node_dependency: mark a test that requires node.
ruby_dependency: mark a test that requires ruby.
go_dependency: mark a test that requires go.
Copy link
Member

Choose a reason for hiding this comment

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

thank you

tests/test_go_server.py Outdated Show resolved Hide resolved
glean_parser/go_server.py Show resolved Hide resolved
glean_parser/templates/go_server.jinja2 Outdated Show resolved Hide resolved
glean_parser/templates/go_server.jinja2 Show resolved Hide resolved
@dmueller dmueller requested a review from badboy January 16, 2024 16:26
Co-authored-by: Jan-Erik Rediger <jrediger@mozilla.com>
@badboy badboy enabled auto-merge (rebase) January 17, 2024 10:11
@badboy badboy merged commit 61bb44e into mozilla:main Jan 17, 2024
8 checks passed
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.

3 participants