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

extend go_server outputter to support custom pings #758

Merged

Conversation

dmueller
Copy link
Contributor

@dmueller dmueller commented Oct 11, 2024

Problem

https://mozilla-hub.atlassian.net/browse/AE-576

For our server integration in go we would like to use custom pings, as this will give more flexibility into how the data gets organized in BQ when we have more than one ping.

Solution

Adjust error message scenarios in go_server so that it is OK for pings to be defined, and OK if the events ping is not used. The go_server outputter now suppports only events ping, only custom ping(s), or both.

Added more test yaml configuration for each of these three scenarios, since they emit different methods in the templated code.

Enabled boolean support, since the code was already present to support booleans-as-strings in event extra; so now we will also support boolean metric values as well.

Added "time" to test.go.tmpl so that in the unit tests we can populate the datetime field with a value instead of leaving it with a default.

Testing

make test-full

I haven't run this in a place that would actually ingest the logged output, but have run the code similar to how the unit test runs test.go to see a log statement printed to stdout. This would be helpful to confirm that the statement is formatted as expected.

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
    • make test-full runs without any test failures
  • 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 marked this pull request as ready for review October 11, 2024 21:18
@dmueller dmueller requested review from akkomar and a team as code owners October 11, 2024 21:18
@dmueller dmueller requested review from travis79 and removed request for a team October 11, 2024 21:18
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.

Regarding the template: instead of treating events ping as special case, can we generalize the current approach to custom pings?
This would reduce code duplication and make Go template consistent with others (https://github.com/mozilla/glean_parser/blob/main/glean_parser/templates/javascript_server.jinja2, https://github.com/mozilla/glean_parser/blob/main/glean_parser/templates/python_server.jinja2)

…nts support to all the pings. removed the assumption of a log statement can only have a single event in it
…be consistent with other server implementations
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.

LGTM, my only suggestions are about docs and comments to make them (hopefully) clearer.

I haven't run this in a place that would actually ingest the logged output, but have run the code similar to how the unit test runs test.go to see a log statement printed to stdout. This would be helpful to confirm that the statement is formatted as expected.

Last two tests in test_go_server.py validate output against Glean schema. I have also tested this manually with ingestion Decoder and haven't seen any issues.

glean_parser/templates/go_server.jinja2 Outdated Show resolved Hide resolved
glean_parser/templates/go_server.jinja2 Outdated Show resolved Hide resolved
glean_parser/templates/go_server.jinja2 Outdated Show resolved Hide resolved
glean_parser/go_server.py Outdated Show resolved Hide resolved
glean_parser/go_server.py Outdated Show resolved Hide resolved
glean_parser/templates/go_server.jinja2 Outdated Show resolved Hide resolved
@akkomar akkomar merged commit c32ce04 into mozilla:main Oct 21, 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