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

Add logstorage exclusion feature #548

Closed
wants to merge 2 commits into from
Closed

Add logstorage exclusion feature #548

wants to merge 2 commits into from

Conversation

ranoongs
Copy link
Contributor

@ranoongs ranoongs commented Oct 5, 2023

This commit is to add 'exclusion' feature in logstorage.
It can be used when taking in all the DLT logs and exclude some modules (using wildcards to App ID and Context ID in the filter).

@lvklevankhanh @michael-methner
Could you review this one please?

@minminlittleshrimp
Copy link
Collaborator

Hello @ranoongs
We must not use wildcards for both App and Ctx at the same time. This is, in fact, our dlt mechanism to use only EcuID in this case to avoid mismatch and avoid the complexity of App:Ctx matching.
Could you kindly rework this PR under the concept of valid dlt mechanism?
Thanks

@minminlittleshrimp
Copy link
Collaborator

FYI.
You could refer to the last comment in this PR #442

@ranoongs
Copy link
Contributor Author

ranoongs commented Oct 6, 2023

@minminlittleshrimp
Thanks for the reply!

If there are many [ App ID : Context ID ] pairs (like 40~50) that are logging and if the user wants all the logs except some modules,

[FILTER1]
LogAppName=.*
LogAppNameExclue=APP3,APP4,APP17
ContextName=.*
LogLevel=DLT_LOG_ERROR
File=Test
FileSize=250000
NOFiles=5
EcuID=ECU1

could be useful rather than below

[FILTER1]
LogAppName=APP1,APP2,APP5,APP6,APP7, ... ,AP16,AP18, ... , AP49,AP50
ContextName=.*
LogLevel=DLT_LOG_ERROR
File=Test
FileSize=250000
NOFiles=5
EcuID=ECU1

And also, my filter generates less keyS so it can save CPU usage as well.

Can't it be considered?

@minminlittleshrimp
Copy link
Collaborator

Hello @ranoongs ,
the point is, if you want to have multi apps + multi ctxs in the filter, you only just need to set EcuID. It make sense here since multi apps+multi ctxs = all logs in Ecu.
Regards

@minminlittleshrimp
Copy link
Collaborator

Your idea is great, but the point here is, it must not be both wildcards app+ctx.
The correct filter will be like this:
[FILTER1]
LogAppName=.*
LogAppNameExclue=APP3,APP4,APP17
ContextName=.*
LogLevel=DLT_LOG_ERROR
File=Test
FileSize=250000
NOFiles=5
EcuID=ECU1 -> this field is enough

@ranoongs
Copy link
Contributor Author

ranoongs commented Oct 7, 2023

@minminlittleshrimp
Oh, I thought the wildcards were needed. Fortunately, my commit does not effect the right form of the filter.
The correct filter as below works fine.

[FILTER1]
LogAppNameExclude=APP3,APP4,AP17
LogLevel=DLT_LOG_DEBUG
File=testlog
FileSize=25000
NOFiles=5
EcuID=ECU1

@minminlittleshrimp
Copy link
Collaborator

Thank you @ranoongs
I will spend time review and run some smoke tests from my side

@ranoongs
Copy link
Contributor Author

@minminlittleshrimp
Any progress?

@minminlittleshrimp
Copy link
Collaborator

Hello @ranoongs
sorry for late response.

Currently we are focusing on some internal DLT issues.
We will get back to your PR asap.

Regards

@minminlittleshrimp
Copy link
Collaborator

Hello @ranoongs
recently I check and we also have this feature, called Negative filter, in our internal dlt version.
I would like to upstream our implementation as we treat them (apid exclude, ctid exclude) as fixed fields in logstorage filter and do not have to turn option on/off.
Also we build the unittest for this feature.
I will open other PR and link to yours.

Regards

@ranoongs
Copy link
Contributor Author

@minminlittleshrimp
Oh I see. I was late!
Then should I delete this PR or just leave it?

@minminlittleshrimp
Copy link
Collaborator

No, you are not late 😀
In fact this idea is so good, we all realize that there is the need for excluded fields sooner or later.
However, dlt team would like this feature to be fixed for filter, not just an option.
I then would like to keep this open until the upstream version is approved and merged.
Thank you for your contribution to dlt.
Regards

@ranoongs
Copy link
Contributor Author

@minminlittleshrimp
Oh! Then could you give me some guidance please?

Do I fix this with fixed feature and wait for the upstream version is merged?

@minminlittleshrimp
Copy link
Collaborator

Please wait for the upstream.
I will close this PR after that.
Thank you

@ranoongs
Copy link
Contributor Author

Sorry I'm a little bit confused.
So the same feature is already in internal DLT (and will be merged) so this PR is unnecessary right?

@minminlittleshrimp
Copy link
Collaborator

Yes @ranoongs

@ranoongs
Copy link
Contributor Author

Okay I remove this PR and I will contribute in other ways. Thanks.

@ranoongs ranoongs closed this Oct 19, 2023
@ranoongs ranoongs deleted the exclusion_contribute branch October 19, 2023 11:58
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.

2 participants