feat: enable sampling for stdout logger #892
Conversation
logger/logrus.go
Outdated
| if shouldDrop(uint(rate)){ | ||
| return | ||
| } | ||
| l.entry.WithField("sample_rate", rate) |
There was a problem hiding this comment.
Let's call this SampleRate so that it's compatible with what happens in Beelines; that way if someone eventually sends these logs to honeycomb they'll be handled properly.
There was a problem hiding this comment.
Oh, I'm not familiar with Beelines. Do you mind to explain a bit more on what the incompatibility looks like here?
There was a problem hiding this comment.
Whenever Honeycomb receives data in a column called SampleRate, we use that in calculations as the sample rate. It's how we compensate for sampling. So we should name the field the way Honeycomb expects to receive it, in case that data is ever forwarded to us.
| - name: SamplerEnabled | ||
| type: bool | ||
| valuetype: nondefault | ||
| default: false | ||
| reload: false | ||
| summary: controls whether logs are sampled before sending to stdout. | ||
| description: > | ||
| The sample rate is controlled by the `SamplerThroughput` setting. | ||
|
|
||
| - name: SamplerThroughput | ||
| valuetype: showexample | ||
| type: float | ||
| default: 10 | ||
| example: 10 | ||
| reload: false | ||
| summary: is the sampling throughput for logs in events per second. | ||
| description: > | ||
| The sampling algorithm attempts to make sure that the average | ||
| throughput approximates this value, while also ensuring that all | ||
| unique logs arrive at stdout at least once per sampling | ||
| period. |
There was a problem hiding this comment.
Is there a reason for using a ">" and the newline before this description?
There was a problem hiding this comment.
Oh I see, this formatting is consistent for this document. Please disregard.
There was a problem hiding this comment.
That's a YAML feature that means this is the start of a string that is a paragraph. A line with a smaller indent ends it.
config/file_config.go
Outdated
| @@ -276,6 +276,8 @@ type HoneycombLoggerConfig struct { | |||
|
|
|||
| type StdoutLoggerConfig struct { | |||
| Structured bool `yaml:"Structured" default:"false"` | |||
| SamplerEnabled bool `yaml:"SamplerEnabled" ` | |||
| SamplerThroughput int `yaml:"SamplerThroughput" default:"5"` | |||
There was a problem hiding this comment.
This SamplerThroughput default value conflicts with the default set in configMeta.yaml.
There was a problem hiding this comment.
It also conflicts with the stated default value in the generated documentation.
There was a problem hiding this comment.
Good find! I honestly don't really understand the relationship here. I got the default from line 274, which is the config for HoneycombLogger. It looks like that one also has the same issue.
@kentquirk , do you mind to explain a bit more on the relationship between each default values?
There was a problem hiding this comment.
Ok, I took a look of the config logic and here's my understanding. Please correct me if I'm wrong in any way @kentquirk. I believe the ones in metadata is purely for documentation and validation logic. It looks the default value defined in the yaml tag is the one that's being loaded and used.
I modified the metadata file to match with this value
There was a problem hiding this comment.
Awesome! Then everything looks good to me.
There was a problem hiding this comment.
Please see #894 -- turns out there were a few of these.
Which problem is this PR solving?
#705
Short description of the changes
This PR implements the same sampling logic from HoneycombLogger in the StdoutLogger.