Allow users to disable event serialization if metrics are empty

[jaredcnance]

Issue #, if available: Closes #51

Introduce a new configuration flag ONLY_LOG_EVENTS_WITH_METRICS which will allow users to disable serialization unless metrics are present. The reason this is put behind a flag is because some users may still expect events even if there are no metrics added. Another option would be to extend this to properties/dimensions as well so that we only skip serialization if the logger has not been updated at all.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[Jeff17Robbins]

@jaredcnance, the description is somewhat confusing for a newbie (me)

ONLY_LOG_EVENTS_WITH_METRICS: When this property is set to true, LogEvents without any metrics set will be dropped.

I think it is written with the "internal knowledge" about LogEvents. But in my stumbling upon the issue, I didn't even know about LogEvents.

I used the decorator @metric_scope on my function, and simply never had reason to call metrics.<XYZ> in my function.

The decorator seemed designed to make things simple, so I wouldn't necessarily know that simply by decorating, I was in the category of calling "LogEvents without any metrics set".

[jaredcnance]

@Jeff17Robbins thanks for the feedback. Actually in this context it LOG_EVENT was intended to be a verb-subject (an alternate form might be EMIT_EVENT). combination rather than a reference to the LogEvent model. But, the confusion is understandable. Maybe:

• ONLY_EMIT_EVENTS_WITH_METRICS (default: false)
• SKIP_EVENTS_WITH_NO_METRICS (default: false)
• DISCARD_EVENTS_WITHOUT_METRICS (default: false)

One option, is to change the default behavior so that if no metrics have been added, we don't emit anything by default. The problem is that I believe there are still use cases where you might want the events, even if it is just a record that the operation happened. It might be unexpected that if I remove a call to put_metric that I would lose the record of an event even happening.

[Jeff17Robbins]

@jaredcnance if there are no metrics (and, say, no custom properties either), what is the use-case for the "empty" record in the log? I'm genuinely curious, since until now I was perceiving this simply as a bug, not as a feature! I don't think I've seen any examples of what it is for, or why someone might want it, which played a role in my perception.

In reading the EMF spec, the definition of CloudWatchMetrics is potentially ambiguous wrt being empty, since it is pictured this way

{
  "_aws": {
    "CloudWatchMetrics": [ ... ]
  }
}

where the ... perhaps implies non-empty? But if an empty CloudWatchMetrics array is useful to folks, I'm fine with opting out of that, as a config option like you suggested. Of the 3 choices, I think ONLY_EMIT_EVENTS_WITH_METRICS is the best.

Perhaps ONLY_LOG_EVENTS_WITH_METRICS (verb_noun) as you had it is better still, now that I understand the topic better. I don't mean to irritate by going back to your original idea!

[jaredcnance]

The use case might be that the event or invocation ever happened. You get a lot of metadata included in each event which might be valuable even if no metrics have been added. Now that being said, you raise a good point. If no metrics have been added we do not need to include the _aws field and we can drop it here:

aws-embedded-metrics-python/aws_embedded_metrics/serializers/log_serializer.py

Line 42 in 3e5ccce

if not config.disable_metric_extraction:

I don’t believe there is any use case for this section if no metrics have been added. I will update the PR to skip adding this field if no metrics have been added.

There is some overlap with this proposal and #57. I think an implementation for conditional filtering could support this use case without the need for an additional config flag. However, right now I think for now this is a relatively simple solution that is worth shipping by itself. Not sure if @benkehoe had any thoughts on this.