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.

Sign up for GitHub

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

Jump to bottom

[extension/ackextension] Create Ack extension interface #31225

Merged
merged 15 commits into from
Feb 22, 2024
Merged

[extension/ackextension] Create Ack extension interface #31225

merged 15 commits into from
Feb 22, 2024

Conversation

zpzhuSplunk
Copy link
Contributor

@zpzhuSplunk zpzhuSplunk commented Feb 13, 2024 •
edited
Loading

Description:
Adding interface and other related files to serve as placeholder for the Ack extension proposed in #26376. Implementation will be added after this PR gets merged.

Link to tracking Issue:
#26376

Testing:

Documentation:

@zpzhuSplunk zpzhuSplunk requested review from a team and atoulme February 13, 2024 16:56
@zpzhuSplunk zpzhuSplunk force-pushed the ack_extension_interface branch from 2969cc9 to b6f26bc Compare February 13, 2024 17:28
splunkericl
splunkericl reviewed Feb 13, 2024
View reviewed changes
extension/ackextension/config.go Show resolved Hide resolved
extension/ackextension/ackextension.go Show resolved Hide resolved
extension/ackextension/ackextension.go Outdated Show resolved Hide resolved
@splunkericl
Copy link
Contributor

Hey @atoulme , we are thinking of breaking the change into 3 parts:

  1. add interface, factory and skeleton files
  2. add extension implementation
  3. use extension inside hec receiver

Do you think this will be the right approach to contribute this piece? Or do you prefer to combine everything in one shot?

@zpzhuSplunk zpzhuSplunk force-pushed the ack_extension_interface branch 3 times, most recently from affe03a to 4f36854 Compare February 14, 2024 01:02
@github-actions github-actions bot added the cmd/otelcontribcol otelcontribcol command label Feb 14, 2024
@zpzhuSplunk zpzhuSplunk force-pushed the ack_extension_interface branch 4 times, most recently from d2653c7 to 3c8cb75 Compare February 14, 2024 22:58
@dmitryax
Copy link
Member

we are thinking of breaking the change into 3 parts

Right, please follow this approach

@zpzhuSplunk zpzhuSplunk force-pushed the ack_extension_interface branch from 3c8cb75 to 8bfc5b3 Compare February 15, 2024 04:31
@zpzhuSplunk zpzhuSplunk force-pushed the ack_extension_interface branch 3 times, most recently from 479eba2 to 82ed939 Compare February 15, 2024 05:41
dmitryax
dmitryax reviewed Feb 15, 2024
View reviewed changes
.github/CODEOWNERS Outdated
@@ -81,6 +81,7 @@ exporter/syslogexporter/ @open-telemetry/collect
exporter/tencentcloudlogserviceexporter/ @open-telemetry/collector-contrib-approvers @wgliang @yiyang5055
exporter/zipkinexporter/ @open-telemetry/collector-contrib-approvers @MovieStoreGuy @astencel-sumo @crobert-1

extension/ackextension/ @open-telemetry/collector-contrib-approvers @atoulme
Copy link
Member

Choose a reason for hiding this comment

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

We need more code owners from your side. This requires OTel org membership. @splunkericl I see you have made enough contributions to apply for membership. Can you submit an issue similar to open-telemetry/community#1915?

Copy link
Member

Choose a reason for hiding this comment

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

@zpzhuSplunk @splunkericl, while the component is in the development phase, I believe you should be able to add yourselves in the code owner list even without the Opentlemetry membership

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure we can.

@zpzhuSplunk zpzhuSplunk force-pushed the ack_extension_interface branch from f264937 to 6ce6511 Compare February 16, 2024 21:26
dmitryax
dmitryax approved these changes Feb 21, 2024
View reviewed changes
Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

LGTM. Just needs updating .github/CODEOWNERS

fatsheep9146
fatsheep9146 reviewed Feb 21, 2024
View reviewed changes
extension/ackextension/README.md Outdated
```yaml
extensions:
ack:
storagetype: "in-memory"
Copy link
Contributor

Choose a reason for hiding this comment

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

small nit, I think we'd better keep the indent consistent

extensions:
  ack:
    storagetype: "in-memory"

receivers:
  splunk_hec:
    ack_extensions: ack

Copy link
Member

Choose a reason for hiding this comment

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

Right. Please also update it to use storage instead of storagetype

@fatsheep9146
Copy link
Contributor

@zpzhuSplunk
bdc786b
sorry for this commit, I just want to make a comments but made a commit instead, feel free to revert it.

@zpzhuSplunk zpzhuSplunk force-pushed the ack_extension_interface branch 2 times, most recently from 5e6d203 to 72005aa Compare February 21, 2024 09:30
@codecov Codecov
Copy link

codecov bot commented Feb 21, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (5f81014) 82.02% compared to head (72005aa) 82.02%.
Report is 13 commits behind head on main.

Files Patch % Lines
extension/ackextension/factory.go 84.61% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #31225      +/-   ##
==========================================
- Coverage   82.02%   82.02%   -0.01%     
==========================================
  Files        1860     1863       +3     
  Lines      171900   172014     +114     
==========================================
+ Hits       140998   141087      +89     
- Misses      26716    26733      +17     
- Partials     4186     4194       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zpzhuSplunk zpzhuSplunk force-pushed the ack_extension_interface branch from b00dddc to cc9d174 Compare February 21, 2024 17:55
dmitryax
dmitryax reviewed Feb 21, 2024
View reviewed changes
extension/ackextension/README.md Outdated
```yaml
extensions:
ack:
storageID:
Copy link
Member

@dmitryax dmitryax Feb 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Let's use storage as defined in the config.go

Suggested change
storageID:
storage:

extension/ackextension/README.md Outdated

receivers:
splunk_hec:
ack_extensions: ack
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ack_extensions: ack
ack_extension: ack

@fatsheep9146 fatsheep9146 added the ready to merge Code review completed; ready to merge by maintainers label Feb 22, 2024
@dmitryax dmitryax merged commit 449b6a4 into open-telemetry:main Feb 22, 2024
147 checks passed
@github-actions github-actions bot added this to the next release milestone Feb 22, 2024
@splunkericl splunkericl mentioned this pull request Mar 1, 2024
Closed
6 tasks
XinRanZhAWS pushed a commit to XinRanZhAWS/opentelemetry-collector-contrib that referenced this pull request Mar 13, 2024
@zpzhuSplunk @XinRanZhAWS
[extension/ackextension] Create Ack extension interface (open-telemet…
9e5ce5f 
…ry#31225)

**Description:**
Adding interface and other related files to serve as placeholder for the
Ack extension proposed in
open-telemetry#26376.
Implementation will be added after this PR gets merged.

**Link to tracking Issue:**

open-telemetry#26376
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timannguyen timannguyen timannguyen left review comments

@splunkericl splunkericl splunkericl left review comments

@fatsheep9146 fatsheep9146 fatsheep9146 approved these changes

@dmitryax dmitryax dmitryax approved these changes

@atoulme atoulme Awaiting requested review from atoulme

Assignees

@fatsheep9146 fatsheep9146

Labels
cmd/otelcontribcol otelcontribcol command ready to merge Code review completed; ready to merge by maintainers reports/distributions/contrib.yaml reports/distributions/splunk.yaml
Projects
None yet
Milestone
v0.96.0
Development

Successfully merging this pull request may close these issues.
5 participants
@zpzhuSplunk @splunkericl @dmitryax @fatsheep9146 @timannguyen