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

[RFC] report: encode timestamps in a compact form #2440

Closed

Conversation

alban
Copy link
Contributor

@alban alban commented Apr 13, 2017

This patch changes the encoding of two kinds of timestamps:

  • timestamps in the "latest" map
  • timestamps in metrics

We use Golang's internal time.Time marshalling and then base64 on it.

Before this patch, this is how it looked:

"host_node_id" : {
   "value" : "neptune;<host>",
   "timestamp" : "2017-04-07T15:50:55.320041355Z"
},
...
"samples" : [
   {
      "date" : "2017-04-07T17:50:40.284167472+02:00",
      "value" : 249266176
   },

After this patch, this is how it looks:

"latest" : {
   "host_node_id" : {
      "t" : "AQAAAA7QgZARDlWbvv//",
      "value" : "neptune;<host>"
   },
...
"samples" : [
   {
      "v" : 24.330900243309,
      "t" : "AQAAAA7QgZANEJKTv///"
   },
   {
      "v" : 25.9842519685039,
      "t" : "AQAAAA7QgZAODml7Af//"
   },

On my laptop with one Docker container running and 392 processes
running, I see 17815 timestamps in the report.

This should make the report smaller in its uncompressed form.

However, I should check if it makes the reports bigger in its compressed
form. It might happen because base64 strings are more difficult to
compress. I have not tested this yet. See:
http://stackoverflow.com/questions/38124361/why-does-base64-encoded-data-compress-so-bad


Addresses bug #1201

TODO:

  • compare the report size (both compressed and uncompressed)
  • check backward compatibility
  • check cpu impact

/cc @ekimekim @bboreham

  • Do you think it worth the complexity?

This patch changes the encoding of two kinds of timestamps:
- timestamps in the "latest" map
- timestamps in metrics

We use Golang's internal time.Time marshalling and then base64 on it.

Before this patch, this is how it looked:
```json
"host_node_id" : {
   "value" : "neptune;<host>",
   "timestamp" : "2017-04-07T15:50:55.320041355Z"
},
...
"samples" : [
   {
      "date" : "2017-04-07T17:50:40.284167472+02:00",
      "value" : 249266176
   },
```

After this patch, this is how it looks:
```json
"latest" : {
   "host_node_id" : {
      "t" : "AQAAAA7QgZARDlWbvv//",
      "value" : "neptune;<host>"
   },
...
"samples" : [
   {
      "v" : 24.330900243309,
      "t" : "AQAAAA7QgZANEJKTv///"
   },
   {
      "v" : 25.9842519685039,
      "t" : "AQAAAA7QgZAODml7Af//"
   },
```

On my laptop with one Docker container running and 392 processes
running, I see 17815 timestamps in the report.

This should make the report smaller in its uncompressed form.

However, I should check if it makes the reports bigger in its compressed
form. It might happen because base64 strings are more difficult to
compress. I have not tested this yet. See:
http://stackoverflow.com/questions/38124361/why-does-base64-encoded-data-compress-so-bad
@alban alban force-pushed the alban/timestamp-encode branch from d247f51 to b4a5628 Compare April 13, 2017 16:13
@bboreham
Copy link
Collaborator

Where are you seeing json reports? I thought all the ones used for real are msgpack now.

@bboreham
Copy link
Collaborator

and no, I don't think this is worth it, sorry

@ekimekim
Copy link
Contributor

Agreed. Unless there's a visible perf improvement, this isn't worth the extra complexity. For one thing, it irrevocably ties the report format to golang, which matters for people writing plugins.

@alban
Copy link
Contributor Author

alban commented Apr 18, 2017

Where are you seeing json reports? I thought all the ones used for real are msgpack now.

Yes, it uses msgpack. I just got the snippets in json form by clicking on the "download json" debug button in the Scope UI.

Fair enough, I'm closing this PR.

@alban alban closed this Apr 18, 2017
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