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

Logs volume histogram support #343

Closed
wants to merge 19 commits into from
Closed

Conversation

slvrtrn
Copy link
Contributor

@slvrtrn slvrtrn commented Mar 31, 2023

I cannot create a PR on top of an external branch (see #328), so this one contains both changesets.

To make it simpler for reviewing, here is an identical PR based on my fork where I could use #328 as a base branch: https://github.com/slvrtrn/clickhouse-datasource/pull/1/files

Screenshots:

g1

g2

For obvious reasons, I added two new fields if the format is set to LOGS and the builder mode is Table - timeField and logLevelField. The only thing I could not figure out is how to restrict showing these fields only when the Explore mode is selected.

I used core ElasticSearch implementation as a reference, and there is a fair amount of copy-paste from the base repository (mainly from here).

If we could get

  • LogLevelColor
  • getThemeColor
  • getLogVolumeFieldConfig
  • getIntervalInfo

as a part of the public API, the amount of copy-pasted code will be significantly reduced.

Of course, some tests are in order, but I wanted to collect some general feedback and maybe some insights considering the TODOs in the code.

It works the following way:

  • In Explore, if LOGS format is selected and the mode is Table (List), we see two new fields - timeField and logLevelField. If the timeField is set, the supplementary query for logs volumes will be generated and executed.
  • Assuming that timeField is set to created_at, if logLevelField is set, the generated query looks like this (abbreviations are taken from LogLevel enum):
SELECT sum(toString("level") IN
           ('critical', 'fatal', 'crit', 'alert', 'emerg', 'CRITICAL', 'FATAL', 'CRIT', 'ALERT', 'EMERG', 'Critical',
            'Fatal', 'Crit', 'Alert', 'Emerg'))                                                           AS critical,
       sum(toString("level") IN ('error', 'err', 'eror', 'ERROR', 'ERR', 'EROR', 'Error', 'Err', 'Eror')) AS error,
       sum(toString("level") IN ('warn', 'warning', 'WARN', 'WARNING', 'Warn', 'Warning'))                AS warn,
       sum(toString("level") IN
           ('info', 'information', 'informational', 'INFO', 'INFORMATION', 'INFORMATIONAL', 'Info', 'Information',
            'Informational'))                                                                             AS info,
       sum(toString("level") IN ('debug', 'dbug', 'DEBUG', 'DBUG', 'Debug', 'Dbug'))                      AS debug,
       sum(toString("level") IN ('trace', 'TRACE', 'Trace'))                                              AS trace,
       sum(toString("level") IN ('unknown', 'UNKNOWN', 'Unknown'))                                        AS unknown,
       toStartOfInterval("created_at", INTERVAL 1 DAY)                                                    AS time
FROM default."logs"
WHERE (created_at >= $__fromTime AND created_at <= $__toTime)
GROUP BY toStartOfInterval("created_at", INTERVAL 1 DAY) AS time
ORDER BY time ASC;

NB: here I am avoiding lower function usage, so reasonable values for IN clauses are generated in advance.

  • If logLevelField is NOT set, the generated query looks like this:
SELECT toStartOfInterval("created_at", INTERVAL 1 MINUTE) AS time, count(*) logs
FROM default."logs"
WHERE (created_at >= $__fromTime AND created_at <= $__toTime)
GROUP BY toStartOfInterval("created_at", INTERVAL 1 MINUTE) AS time
ORDER BY time ASC;
  • In the end, it gives us a single data frame with time plus all the log-level fields, which are transformed into a data frame per level.

@slvrtrn slvrtrn requested a review from a team as a code owner March 31, 2023 00:41
@gingerwizard
Copy link
Collaborator

gingerwizard commented Mar 31, 2023

Firstly, this rocks. 👏

Questions:

  1. How will this work in plain SQL view? - this is how i expected most users will interact with Logs.
  2. We added the format to address some challenges with traces and logs. The Format conflicts somewhat with the Show as but i think they are complementary i.e. they let someone build an aggregation but render as logs. Show As should probably thus be Build as. If this work is going into a 3x release (due to SDK update), we should take the opportunity to clean this all up.
  3. If format is to Logs, there is backend code that auto-detects the time field - the first one. Confusingly though also, the front end sets the format if the log_time column is present (it does this for timeseries as well) - the explicit format overrides this detection. I think the intention of this was for the SQL view. If we add the explicit options to set the log time column we should remove any auto-detection IMO in the frontend.
  4. The elasticsearch plugin requires the time field to be set at the datasource for example. Did we consider this? it might help with (1). Users could also set the log level here.
  5. The bucket time width should be dynamic based on the interval - see how the macro $__timeInterval works.

@bossinc
Copy link
Collaborator

bossinc commented Mar 31, 2023

Drone is failing on yarn spellcheck Please add these words to cspell.config.json

 67/82 ./src/data/CHDatasource.ts 57.35ms X
/drone/src/src/data/CHDatasource.ts:71:15 - Unknown word (timespan)
/drone/src/src/data/CHDatasource.ts:72:76 - Unknown word (timespan)
/drone/src/src/data/CHDatasource.ts:89:102 - Unknown word (timespan)
/drone/src/src/data/CHDatasource.ts:119:5 - Unknown word (timespan)
/drone/src/src/data/CHDatasource.ts:134:7 - Unknown word (timespan)

68/82 ./src/data/logs.ts 47.05ms X
/drone/src/src/data/logs.ts:181:57 - Unknown word (timespan)
/drone/src/src/data/logs.ts:186:9 - Unknown word (timespan)
/drone/src/src/data/logs.ts:211:3 - Unknown word (timespan)
/drone/src/src/data/logs.ts:218:9 - Unknown word (timespan)
/drone/src/src/data/logs.ts:241:53 - Unknown word (emerg)
/drone/src/src/data/logs.ts:242:28 - Unknown word (eror)
/drone/src/src/data/logs.ts:245:21 - Unknown word (dbug)

@mshustov
Copy link
Collaborator

mshustov commented Apr 3, 2023

Adding a filter when clicking on the amplifier on Logs view doesn't work. @slvrtrn is it something we're going to add in a follow-up?
2023-04-03_20-38-48

In Explore, if LOGS format is selected and the mode is Table (List), we see two new fields - timeField and logLevelField.

Note that Log level field and Time field aren't reset when changing a table name in Query builder

How will this work in plain SQL view? - this is how i expected most users will interact with Logs.

@gingerwizard We can add Time field and Log level field input elements. But I'm not opposed to (4) aligning with elaticsearch plugin behavior, which might be already well-known among users.

The bucket time width should be dynamic based on the interval - see how the macro $__timeInterval works.

@gingerwizard Could you elaborate? Don't getIntervalInfo & getTimeFieldRoundingClause implement bucketing?

If we could get
LogLevelColor
getThemeColor
getLogVolumeFieldConfig
getIntervalInfo

@bossinc @asimpson Would Grafana team accept a PR exporting these from https://github.com/grafana/grafana/blob/ab53772416659fa845effe4913051e1f829d47e2/public/app/core/logsModel.ts?
This logic seems to be fundamental for any plugin implementing Logging support

@slvrtrn
Copy link
Contributor Author

slvrtrn commented Apr 3, 2023

Adding a filter when clicking on the amplifier on Logs view doesn't work. @slvrtrn is it something we're going to add in a follow-up?

Yes

src/data/CHDatasource.ts Outdated Show resolved Hide resolved
src/data/CHDatasource.ts Outdated Show resolved Hide resolved
.config/webpack/webpack.config.ts Outdated Show resolved Hide resolved
src/data/CHDatasource.ts Show resolved Hide resolved
const fields: string[] = [];
const metrics: BuilderMetricField[] = [];
// could be undefined or an empty string (if user deselects the field)
if (query.builderOptions.logLevelField) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I'd extract it to a separate function and add some tests for it.

src/data/CHDatasource.ts Show resolved Hide resolved
import { partition } from 'lodash';

/**
* Partially copy-pasted and adjusted to ClickHouse server-side aggregations
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nevertheless, can we add some tests?
@slvrtrn Can we benefit from Grafana exporting this functionality?

src/components/queryBuilder/QueryBuilder.tsx Show resolved Hide resolved
src/components/queryBuilder/LogLevelField.tsx Show resolved Hide resolved
.map((f) => ({ label: f.label, value: f.name }));
if (columns.length) {
columns.push({
label: '-',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are there best practices for Null values to reset the selection? @bossinc

export const LogLevelFieldEditor = (props: LogLevelEditorProps) => {
const { label, tooltip } = selectors.components.QueryEditor.QueryBuilder.LOG_LEVEL_FIELD;
// TODO: filter by strings, enums?
const columns: SelectableValue[] = (props.fieldsList || [])
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Could you add a few tests for this logic as well?

slvrtrn and others added 3 commits April 4, 2023 13:42
Co-authored-by: Andrew Hackmann <5140848+bossinc@users.noreply.github.com>
Co-authored-by: Andrew Hackmann <5140848+bossinc@users.noreply.github.com>
@slvrtrn
Copy link
Contributor Author

slvrtrn commented Apr 4, 2023

Let's not merge it yet.

@asimpson
Copy link
Contributor

asimpson commented Apr 5, 2023

Chiming in here re:

  • LogLevelColor
  • getThemeColor
  • getLogVolumeFieldConfig
  • getIntervalInfo

Let's move forward as is and if/when those pieces get exported we can refactor.

@slvrtrn
Copy link
Contributor Author

slvrtrn commented Apr 5, 2023

I've recreated this PR as #352
Let's continue the review there.

@slvrtrn slvrtrn closed this Apr 5, 2023
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.

None yet

5 participants