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

[cisco_ios]: escape hyphen inside regex char list #9566

Closed

Conversation

pkoutsovasilis
Copy link
Contributor

Proposed commit message

After tackling this issue, a respective PR was created to elastic-package to mainstream the detection of similar warnings. Thus this PR fixes the following similar to the former issue regex-related warning for cisco_ios

character class has '-' without escape

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

Author's Checklist

N/A

How to test this PR locally

Related issues

N/A

Screenshots

N/A

@pkoutsovasilis pkoutsovasilis added bug Something isn't working, use only for issues Integration:cisco_ios Cisco IOS Team:Security-Deployment and Devices Deployment and Devices Security team [elastic/sec-deployment-and-devices] labels Apr 11, 2024
@pkoutsovasilis pkoutsovasilis self-assigned this Apr 11, 2024
@pkoutsovasilis pkoutsovasilis force-pushed the pkoutsovasilis/fix_cisco_ios_regex_warnings branch from 79192e4 to c1f034b Compare April 11, 2024 05:56
@pkoutsovasilis pkoutsovasilis changed the title fix(cisco_ios): escape hyphen inside regex char list [cisco_ios]: escape hyphen inside regex char list Apr 11, 2024
@pkoutsovasilis pkoutsovasilis marked this pull request as ready for review April 11, 2024 05:57
@pkoutsovasilis pkoutsovasilis requested a review from a team as a code owner April 11, 2024 05:57
@elasticmachine
Copy link

Pinging @elastic/sec-deployment-and-devices (Team:Security-Deployment and Devices)

@elasticmachine
Copy link

elasticmachine commented Apr 11, 2024

🚀 Benchmarks report

Package cisco_ios 👍(1) 💚(0) 💔(0)

Expand to view
Data stream Previous EPS New EPS Diff (%) Result
log 0 637.76 637.76 ( - %) 👍

Copy link
Contributor

@jrmolin jrmolin left a comment

Choose a reason for hiding this comment

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

one question, not going to block for an answer.

@dwhyrock
Copy link
Contributor

From the report:

Data stream Previous EPS New EPS Diff (%) Result
log 2207.51 545.85 -1661.66 (-75.27%) 💔

Is EPS "events per second"? And should we be concerned with the numbers here?

@pkoutsovasilis
Copy link
Contributor Author

From the report:

Data stream Previous EPS New EPS Diff (%) Result
log 2207.51 545.85 -1661.66 (-75.27%) 💔
Is EPS "events per second"? And should we be concerned with the numbers here?

hmmm not sure to be honest I just escaped a hyphen I didn't expect such a massive impact on perf 🤷‍♂️

@taylor-swanson
Copy link
Contributor

I pulled the changes locally and ran benchmarks myself. I'm not noticing any notable differences between the current version in main and this one. If anything the timing seems slightly tighter and ever so slightly faster with this change than main. Unfortunately, the benchmark tool doesn't give anything more specific for stats (percentiles, standard deviation, etc) so that's all based on "feel".

I'm not sure exactly how the CI runs the benchmarks, but if it's only once, then unfortunately there's a chance that it has a bad run and show a large "change", when in reality it could run it again and show that it's the same or faster.

@pkoutsovasilis
Copy link
Contributor Author

/test benchmark fullreport

@pkoutsovasilis pkoutsovasilis force-pushed the pkoutsovasilis/fix_cisco_ios_regex_warnings branch from 438701e to 0ffda87 Compare April 12, 2024 08:28
@pkoutsovasilis
Copy link
Contributor Author

pkoutsovasilis commented Apr 12, 2024

So @taylor-swanson @dwhyrock @jrmolin, after having a look at the CI steps, the elastic-package benchmark is invoked only once here. The baseline benchmark results are uploaded from main, so when a commit is merged the affected packages are detected and for ones found, benchmark is run and pushed in the respective buildkite artifact. That said, I did the same practice as Taylor in his comments and my local running benchmark report is not exhibiting such a substantial perf difference. However, I did push a commit that reverts the fix and just does a dummy change to cause the benchmarking to execute and the performance hindering is gone 🤷‍♂️ Any more ideas?

@pkoutsovasilis
Copy link
Contributor Author

/test benchmark fullreport

@elasticmachine
Copy link

💚 Build Succeeded

History

cc @pkoutsovasilis

Copy link

@pkoutsovasilis
Copy link
Contributor Author

🚀 Benchmarks report

Package cisco_ios 👍(1) 💚(0) 💔(0)

Expand to view
Data stream Previous EPS New EPS Diff (%) Result
log 0 637.76 637.76 ( - %) 👍

I think somehow all my tweaking and messing around managed to mess up with the benchmarking. In an attempt to recover that, I am gonna close this one and open a new one with my experiments

Copy link
Contributor

mergify bot commented Apr 12, 2024

⚠️ The sha of the head commit of this PR conflicts with #9578. Mergify cannot evaluate rules on this PR. ⚠️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working, use only for issues Integration:cisco_ios Cisco IOS Team:Security-Deployment and Devices Deployment and Devices Security team [elastic/sec-deployment-and-devices]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants