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

Move openmetrics module to oss #26561

Merged
merged 8 commits into from
Jun 30, 2021

Conversation

ChrsMark
Copy link
Member

What does this PR do?

This PR moves openmetrics Metricbeat module under oss directory.

@ChrsMark ChrsMark added the Team:Integrations Label for the Integrations team label Jun 29, 2021
@ChrsMark ChrsMark self-assigned this Jun 29, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Integrations)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Jun 29, 2021
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jun 29, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #26561 updated

  • Start Time: 2021-06-30T14:56:07.230+0000

  • Duration: 141 min 17 sec

  • Commit: 1f05045

Test stats 🧪

Test Results
Failed 0
Passed 47281
Skipped 5259
Total 52540

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 47281
Skipped 5259
Total 52540

@jsoriano
Copy link
Member

CI failure seems related.

It'd be nice to add a changelog entry for this.

@mergify
Copy link
Contributor

mergify bot commented Jun 29, 2021

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b move_openmetrics_to_oss upstream/move_openmetrics_to_oss
git merge upstream/master
git push upstream move_openmetrics_to_oss

@ChrsMark ChrsMark requested a review from a team as a code owner June 30, 2021 08:37
@botelastic botelastic bot added the Team:Automation Label for the Observability productivity team label Jun 30, 2021
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Jenkinsfile Outdated
Comment on lines 747 to 749
if(!fileExists(module)) {
module = ''
}
Copy link
Contributor

Choose a reason for hiding this comment

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

the getCommonModuleInTheChangeSet(String directory) search for a pattern in a directory path, fi the pattern does not exist will return ''. I am not sure of the goal of check a folder with the name of the module exists in the current folder, probably will never exist.

cc @v1v

Copy link
Member Author

@ChrsMark ChrsMark Jun 30, 2021

Choose a reason for hiding this comment

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

Is pattern checked for the current(=new) state or the old one? In this particular case we have removed the openmetrics module completely from x-pack but the tester still understands that it has changes and thus it kicks the tests for it which leads to test's failure later on since the module does not actually exists under x-pack.

Copy link
Member

Choose a reason for hiding this comment

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

A bit of context:

Unfortunately this is an edge case when moving files between x-pack and oss beats.

elastic/apm-pipeline-library#535 didn't implement a git diff by excluding files that have been deleted/moved.

So somehow it tries to run the module in x-pack when that particular module was moved to the oss.

Therefore, the validation if the module folder exists could be a good workaround.

I am not sure of the goal of check a folder with the name of the module exists in the current folder, probably will never exist.

if the module name exists in the pwd then it should work, unless module is a nested folder somewhere else, if so, it's required to consider that particular case.

Copy link
Member

@v1v v1v Jun 30, 2021

Choose a reason for hiding this comment

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

I've just confirmed what Ivan said, and there is something missing in this logic, something like the below snippet might work,

Suggested change
if(!fileExists(module)) {
module = ''
}
if(!fileExists("${directory}/module/${module}")) {
module = ''
}

though I don't know if module is always a nester folder under <beat_name_folder> or x-pack/<beat_name_folder>

Actually yes:

Co-authored-by: Victor Martinez <victormartinezrubio@gmail.com>
@jsoriano
Copy link
Member

Failing test is flaky, opened issue (#26608) and PR to skip (#26609).

@ChrsMark
Copy link
Member Author

Ah now another flaky test...

@v1v @kuisathaverat are you ok with the change on Jenkinsfile?

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Once the changelog entry is added, ship it!

Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark added the backport-v7.15.0 Automated backport with mergify label Jun 30, 2021
@ChrsMark ChrsMark merged commit 92143fe into elastic:master Jun 30, 2021
mdelapenya added a commit to mdelapenya/beats that referenced this pull request Jul 1, 2021
* master:
  [MetricBeat] [AWS] Fix aws metric tags with resourcegroupstaggingapi paginator (elastic#26385) (elastic#26443)
  Move openmetrics module to oss (elastic#26561)
  Skip flaky test TestFilestreamMetadataUpdatedOnRename (elastic#26609)
  [filebeat][fortinet] Use default add_locale for fortinet.firewall (elastic#26524)
  Enroll proxy settings (elastic#26514)
@ChrsMark
Copy link
Member Author

ChrsMark commented Jul 1, 2021

Wondering why mergify didn't open the backport PR for this 🤔

ChrsMark added a commit to ChrsMark/beats that referenced this pull request Jul 1, 2021
@v1v
Copy link
Member

v1v commented Jul 1, 2021

Wondering why mergify didn't open the backport PR for this 🤔

@ChrsMark , backports are automated with mergify as long as the backport-v7.<number> is added.

<number> can be changing with the upcoming versions and therefore .mergify.yml should have those changes in place.

backport-v7.15 does not exist in the .mergify.yml file.

There is some automation to include new versions in the .mergify.yml, but I'm not sure those PRs were merged by the person who bumped those release versions.

In fact, the PR is open, #26620 so , I'll leave that operation to the release manager in the beats team, either that PR can be merged if possible, or the mergify change to be cherry-picked and push to the master branch.

@v1v
Copy link
Member

v1v commented Jul 1, 2021

I've just merged #26620 and created #26641 to automate the auto-merge.

@ChrsMark
Copy link
Member Author

ChrsMark commented Jul 1, 2021

Thanks @v1v !

v1v added a commit to v1v/beats that referenced this pull request Jul 5, 2021
…stage-failed-within-same-build

* upstream/master: (36 commits)
  Revert "[CI] fight the flakiness with some retry option in the CI only for the Pull Requests (elastic#26617)" (elastic#26704)
  Packaging: linux/armv7 is not supported (elastic#26706)
  Cyberarkpas: Link to official docs on how to setup TLS (elastic#26614)
  Make network_direction, registered_domain and convert processors compatible with ES older than 7.13.0 (elastic#26676)
  Disable armv7 packaging (elastic#26679)
  [Heartbeat] use --params flag for synthetics (elastic#26674)
  Update dependent package to avoid downloading a suspicious file (elastic#26406)
  [mergify] set title and allow bp in any direction (elastic#26648)
  Fix memory leak in SQL helper when database is not available (elastic#26607)
  [CI] fight the flakiness with some retry option in the CI only for the Pull Requests (elastic#26617)
  [mergify] automate PRs that change the backport rules (elastic#26641)
  [Metricbeat] Add Airflow module in xpack (elastic#26220)
  chore: add-backport-next (elastic#26620)
  [metricbeat] Add state_job metricset (elastic#26479)
  CI: jenkins labels are less time consuming now (elastic#26613)
  [MetricBeat] [AWS] Fix aws metric tags with resourcegroupstaggingapi paginator (elastic#26385) (elastic#26443)
  Move openmetrics module to oss (elastic#26561)
  Skip flaky test TestFilestreamMetadataUpdatedOnRename (elastic#26609)
  [filebeat][fortinet] Use default add_locale for fortinet.firewall (elastic#26524)
  Enroll proxy settings (elastic#26514)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v7.15.0 Automated backport with mergify Team:Automation Label for the Observability productivity team Team:Integrations Label for the Integrations team v7.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants