[go server] replace panics with returned errors #777
Conversation
…gured or discarding behavior
| {% for extra, metadata in event.extra_keys.items() %} | ||
| {{ extra|event_extra_name }} {{ metadata.type|go_metric_type }} // {{ metadata.description|clean_string }} | ||
| {% endfor %} | ||
| {% for extra, metadata in event.extra_keys.items() %} |
There was a problem hiding this comment.
This template was mixed tabs & spaces, so all the whitespace change is to get everything on tabs
| func (g GleanEventsLogger) Record{{ ping|ping_type_name }}( | ||
| requestInfo RequestInfo, | ||
| params {{ ping|ping_type_name }}, | ||
| ) { | ||
| var metrics = metrics{ | ||
| {% for metric_type, metrics in metrics_by_type.items() %} | ||
| {% if metric_type != 'event' %} | ||
| "{{ metric_type }}": { | ||
| {% for metric in metrics %} | ||
| {% if metric_type == 'datetime' %} | ||
| requestInfo RequestInfo, | ||
| params {{ ping|ping_type_name }}, | ||
| ) error { | ||
| metrics := metrics{ | ||
| {% for metric_type, metrics in metrics_by_type.items() %} | ||
| {% if metric_type != 'event' %} | ||
| "{{ metric_type }}": { | ||
| {% for metric in metrics %} | ||
| {% if metric_type == 'datetime' %} | ||
| "{{ metric|metric_name }}": params.{{ metric|metric_argument_name }}.Format("2006-01-02T15:04:05.000Z"), | ||
| {% else %} | ||
| "{{ metric|metric_name }}": params.{{ metric|metric_argument_name }}, | ||
| {% endif %} | ||
| {% endfor %} | ||
| }, | ||
| {% endif %} | ||
| {% endfor %} | ||
| } | ||
|
|
||
| events := []gleanEvent{} | ||
| {% if metrics_by_type['event'] %} | ||
| if params.Event != nil { | ||
| {% else %} | ||
| "{{ metric|metric_name }}": params.{{ metric|metric_argument_name }}, | ||
| {% endif %} | ||
| {% endfor %} | ||
| }, | ||
| {% endif %} | ||
| {% endfor %} | ||
| } | ||
|
|
||
| events := []gleanEvent{} | ||
| {% if metrics_by_type['event'] %} | ||
| if params.Event != nil { | ||
| events = append(events, params.Event.gleanEvent()) | ||
| } | ||
| {% endif %} | ||
| g.record("{{ ping }}", requestInfo, metrics, events) | ||
| {% endif %} | ||
| return g.record("{{ ping }}", requestInfo, metrics, events) | ||
| } | ||
|
|
||
| // Record and submit `{{ ping }}` ping omitting user request info | ||
| func (g GleanEventsLogger) Record{{ ping|ping_type_name}}WithoutUserInfo( | ||
| params {{ ping|ping_type_name}}, | ||
| ) { | ||
| g.Record{{ ping|ping_type_name }}(defaultRequestInfo, params) | ||
| params {{ ping|ping_type_name}}, | ||
| ) error { | ||
| return g.Record{{ ping|ping_type_name }}(defaultRequestInfo, params) |
There was a problem hiding this comment.
These two generated methods got their signature changed to return error, and the corresponding return statement
| @@ -4,6 +4,7 @@ import ( | |||
| "glean/glean" | |||
| "os" | |||
| "time" | |||
| /* IMPORTS */ | |||
There was a problem hiding this comment.
Added this to help facilitate the tests that used io.Discard / not setting a Writer on the glean logger. Those confirm that no output is produced with io.Discard and that an error is returned if the Writer is nil
dmueller
changed the title
| {% for extra, metadata in event.extra_keys.items() %} | ||
| {{ extra|event_extra_name }} {{ metadata.type|go_metric_type }} // {{ metadata.description|clean_string }} | ||
| {% endfor %} | ||
| {% for extra, metadata in event.extra_keys.items() %} |
tests/test_go_server.py
Outdated
|
|
||
| print(tmpl_code) |
There was a problem hiding this comment.
TY, that was from getting the new tests working
There was a problem hiding this comment.
+1 for error handling approach.
I hate to be starting this discussion (lol) but this repo generally uses spaces, not tabs, so these files will have them both mixed eventually. I know Go's recommendation is to use tabs, but IIUC gofmt should address that. Is this change useful on your side when working with generated file or is it mostly about consistency across files here?
Mostly for consistency within the file itself, |
Per https://github.com/mozilla/glean_parser/blob/main/.editorconfig#L5-L6 space is default indentation character for these files. |
|
I didn't know about the editorconfig code and am using vscode without a plugin for it, so that's good to know about! |
problem
It is idiomatic in go to return errors rather than panic. Panics can cause applications to exit, which is not the desired result if a glean log can't be emitted for any reason.
solution
Switch the exported functions to return an error. This enables applications that use the glean server implementation to inspect that error return value and take whatever action they deem appropriate.
A couple other things changed in the go server template
:=syntaxtesting
make test-fullalso added tests for
Writer = nilandWriter = io.Discardto verify logs are not printed in those scenariosPull Request checklist
make testruns without emitting any warningsmake lintruns without emitting any errorsCHANGELOG.mdor an explanation of why it does not need one