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

Coralogix processor first PR #33458

Merged

Conversation

galrose
Copy link
Contributor

@galrose galrose commented Jun 10, 2024

Description:
Adding a feature - Adding a feature to create templates (blueprints) from sql queries.
Currently specifically for postgresql and mysql queries.

Link to tracking Issue: #33090

Testing:
currently no tests, will be added in next PR

Documentation:
Added documentation for possible configuration and the usecase of the processor

@galrose galrose requested review from a team and mx-psi June 10, 2024 15:02
Copy link

linux-foundation-easycla bot commented Jun 10, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@crobert-1
Copy link
Member

This PR should have a changelog entry. 👍

Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

Looks good overall. I've left a lot of comments on the README, the goal is to be able to flesh out UI/UX up front, before getting too far into the implementation weeds.

There are also a number of CI/CD failures caused by this PR, so please take a look and resolve when you're able. Let me know if you run into any issues there, happy to help!

processor/coralogixprocessor/README.md Outdated Show resolved Hide resolved
processor/coralogixprocessor/README.md Outdated Show resolved Hide resolved
processor/coralogixprocessor/README.md Outdated Show resolved Hide resolved
processor/coralogixprocessor/README.md Outdated Show resolved Hide resolved
processor/coralogixprocessor/README.md Show resolved Hide resolved
processor/coralogixprocessor/config.go Outdated Show resolved Hide resolved
processor/coralogixprocessor/README.md Outdated Show resolved Hide resolved
processor/coralogixprocessor/doc.go Outdated Show resolved Hide resolved
processor/coralogixprocessor/factory.go Outdated Show resolved Hide resolved
processor/coralogixprocessor/span.go Outdated Show resolved Hide resolved
@mx-psi mx-psi removed their request for review June 13, 2024 10:36
@galrose
Copy link
Contributor Author

galrose commented Jun 13, 2024

Hey @crobert-1, thanks for the review!
I did some changes as you requested and I would like to rerun the CI to take a look if some of the issues were fixed.
Im also not sure if I removed enough from span.go or if I should have removed more code.

Thanks for the help 😄

@galrose galrose requested a review from crobert-1 June 13, 2024 12:55
processor/coralogixprocessor/README.md Show resolved Hide resolved
processor/coralogixprocessor/README.md Outdated Show resolved Hide resolved
processor/coralogixprocessor/README.md Outdated Show resolved Hide resolved
processor/coralogixprocessor/README.md Show resolved Hide resolved
processor/coralogixprocessor/README.md Show resolved Hide resolved
processor/coralogixprocessor/config.go Outdated Show resolved Hide resolved
processor/coralogixprocessor/config.go Outdated Show resolved Hide resolved
processor/coralogixprocessor/config.go Outdated Show resolved Hide resolved
processor/coralogixprocessor/config.go Outdated Show resolved Hide resolved
processor/coralogixprocessor/README.md Outdated Show resolved Hide resolved
@galrose galrose requested a review from crobert-1 June 17, 2024 07:44
@galrose
Copy link
Contributor Author

galrose commented Jun 18, 2024

@crobert-1 can you please help me understand why I don't pass the CI?
I am running all the scripts from the first PR section and they all look fine locally

@crobert-1
Copy link
Member

@crobert-1 can you please help me understand why I don't pass the CI? I am running all the scripts from the first PR section and they all look fine locally

You'll have to look through the different failed actions individually to see why they're failing. From what I can tell they're related to this PR.

One example:

license header checking failed:
./config.go
./config.go
./config.go
./config.go
./factory.go
./factory.go
./factory.go
./factory.go
./span.go
./span.go
./span.go
./span.go
make[2]: *** [../../Makefile.Common:174: checklicense] Error 1
make[1]: *** [Makefile:179: processor/coralogixprocessor] Error 2

This means these files don't have the proper license header. If you look in Makefile.Common you can see what this check is doing. You need to run make addlicense to resolve this.

For this failure:

FAIL: "processor/coralogixprocessor" not included in CODEOWNERS

The command make update-codeowners needs to be run, as described in the CONTRIBUTING doc.

There are more failures that need investigated, but let me know if there's anything you don't understand 👍

@galrose galrose requested a review from crobert-1 July 10, 2024 07:49
@galrose
Copy link
Contributor Author

galrose commented Jul 11, 2024

@atoulme I see I need your approval as well for the code owners 😄

@crobert-1
Copy link
Member

Looks like there are some more check failures:

go.mod/go.sum deps changes detected, please run "make gotidy" and commit the changes in this PR.

There may be more than that, I didn't check thoroughly.

@crobert-1 crobert-1 added the ready to merge Code review completed; ready to merge by maintainers label Jul 29, 2024
@MovieStoreGuy MovieStoreGuy merged commit 18a47f3 into open-telemetry:main Aug 9, 2024
154 checks passed
@github-actions github-actions bot added this to the next release milestone Aug 9, 2024
@@ -165,6 +165,7 @@ pkg/winperfcounters/ @open-teleme

processor/attributesprocessor/ @open-telemetry/collector-contrib-approvers @boostchicken
processor/cumulativetodeltaprocessor/ @open-telemetry/collector-contrib-approvers @TylerHelmuth
processor/coralogixprocessor/ @open-telemetry/collector-contrib-approvers @galrose @crobert-1 @eyalatz @roycald245
Copy link
Member

Choose a reason for hiding this comment

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

Copy link

@roycald245 roycald245 Aug 11, 2024

Choose a reason for hiding this comment

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

How do we fix this? Is this because we are missing in cmd/githubgen/allowlist.txt?

f7o pushed a commit to f7o/opentelemetry-collector-contrib that referenced this pull request Sep 12, 2024
**Description:** <Describe what has changed.>
Adding a feature - Adding a feature to create templates (blueprints)
from sql queries.
Currently specifically for postgresql and mysql queries.

**Link to tracking Issue:** open-telemetry#33090

**Testing:** <Describe what testing was performed and which tests were
added.>
currently no tests, will be added in next PR

**Documentation:** <Describe the documentation added.>
Added documentation for possible configuration and the usecase of the
processor

---------

Co-authored-by: Curtis Robert <[email protected]>
Co-authored-by: Antoine Toulme <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd/githubgen ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants