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

LOG-2770: optimize fluent conf for throughput #1563

Merged
merged 1 commit into from Jul 26, 2022

Conversation

jcantrill
Copy link
Contributor

@jcantrill jcantrill commented Jul 14, 2022

Description

This PR:

Links

based on rudamentary testing of viaq optimizations which enable only features that are being used:

filtering                rate m/s
---------                ---------
no filtering             100,000
as-is                     28,000
remove es_index           35,000   *
remove debug state        30,000
remove prune_labels       28,000
remove flat_labels        31,000   *
remove rename_time        15,000   ?
remove delete emtpy       45,000    
remove openshift.sequenc  30,000
remove handle_undefined   28,000   
remove pipeline_meta      45,000   *
remove formatter          35,000

These changes remove much of the fluent viaq design that predates the use of CRIO where there was no standard incoming record format.
Additionally, the improvemets to the kubernetes_metadata plugin are mostly realized by no longer serializing and loading the meta attached to each record

@jcantrill
Copy link
Contributor Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 14, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 14, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jcantrill

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 14, 2022
@jcantrill
Copy link
Contributor Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 25, 2022
log ${record['err'] || record['msg'] || record['MESSAGE'] || record['log']}
</record>
remove_keys req,res,msg,name,level,v,pid,err
json_fields 'message'
Copy link
Contributor

Choose a reason for hiding this comment

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

previously was 'MESSAGE' in upper case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this comes from 3.11 where we were pulling container logs using the journal driver. It is no longer relevant since we only pull container logs and I have changed the group capture parameters: https://github.com/openshift/cluster-logging-operator/pull/1563/files#diff-cdd5a7217e8d3eeb415a8f7f720e09303a82c2f9225ee7484d8bdabc11901f1eR148

@@ -367,7 +295,7 @@ const ConcatLines string = `
{{define "concatLines" -}}
<filter kubernetes.**>
@type concat
key log
key message
Copy link
Contributor

Choose a reason for hiding this comment

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

just for my understanding, this changing related to the optimize data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does not relate to optimization other then I made modifications to the incoming parsing expression and have removed and modified a number of filter configurations.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks

Timestamp: time.Time{},
Message: "*",
LogType: "application",
Level: "*",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we remove ViaqIndexName?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is only needed for Elasticsearch so I removed it from ingress and moved it to the ES blocks of configuration.

@vparfonov
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 26, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 26, 2022

@jcantrill: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit eb79500 into openshift:release-5.4 Jul 26, 2022
@jcantrill jcantrill deleted the log2770 branch July 27, 2022 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants