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

adds duration_ms in int64 to the logs #4509

Merged
merged 2 commits into from
Aug 6, 2021

Conversation

rhassanein
Copy link
Contributor

@rhassanein rhassanein commented Aug 2, 2021

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Motivation

Currently the logged lines contain a duration field that is encoded as a string with varying units. When ingesting these structured logs into a logging system (eg. elasticsearch), the field is parsed as a string which doesn't allow for range queries.

Example from v0.19.0:

{
      "ts": "2021-08-02T11:38:26.928570654Z",
      "cached": 23731,
      "duration": "2.09562852s",
      "tier": "sv",
      "stage": "main",
      "returned": 1567,
      "type": "monitoring",
      "component": "block.BaseFetcher",
      "msg": "successfully synchronized block metadata",
      "caller": "fetcher.go:476",
      "level": "info",
      "partial": 0
}

Adding a dedicated duration_ms integer field solves this problem.

Example:

{
      "duration": "2.09562852s",
      "duration_ms": "2095",
}

In order to avoid schema conflicts and introducing a breaking change, we're adding a new field rather than changing the existing one.

Changes

Adds an integer duration_ms to the logged lines with the duration field.

Verification

@igorwwwwwwwwwwwwwwwwwwww thanks for co-authoring this PR.

GiedriusS
GiedriusS previously approved these changes Aug 2, 2021
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is interesting, I would've just changed duration to always use a fixed unit but this works too and makes sense 🤗 thx

@GiedriusS GiedriusS enabled auto-merge (squash) August 2, 2021 12:04
@GiedriusS
Copy link
Member

Please rebase your branch on the latest main, the tests should be fixed there.

auto-merge was automatically disabled August 4, 2021 10:33

Head branch was pushed to by a user without write access

@rhassanein
Copy link
Contributor Author

@GiedriusS thanks for reviewing this! I just added a CHANGELOG entry and attempted a main rebase, hopefully that fixes the tests.

Signed-off-by: rhassanein <rhassanein@gitlab.com>
@rhassanein rhassanein changed the title Adds duration_ms in int64 to the logs Draft: adds duration_ms in int64 to the logs Aug 4, 2021
Signed-off-by: rhassanein <rhassanein@gitlab.com>
@rhassanein rhassanein changed the title Draft: adds duration_ms in int64 to the logs adds duration_ms in int64 to the logs Aug 5, 2021
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@bwplotka bwplotka merged commit 1964c0c into thanos-io:main Aug 6, 2021
@vanugrah
Copy link
Contributor

Sweet! I was just thinking about this :D

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.

4 participants