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

Add skeleton for new delta to rate processor #4218

Merged
merged 4 commits into from
Jul 29, 2021
Merged

Add skeleton for new delta to rate processor #4218

merged 4 commits into from
Jul 29, 2021

Conversation

hossain-rayhan
Copy link
Contributor

Signed-off-by: Rayhan Hossain [email protected]

Description:
This metric calculates rate (gauge) from delta (sum).

Link to tracking Issue: #3751

Testing: Unit tests added.

Documentation: README added.

@hossain-rayhan hossain-rayhan requested a review from a team as a code owner July 19, 2021 13:19
@project-bot project-bot bot added this to In progress in Collector Jul 19, 2021
@hossain-rayhan
Copy link
Contributor Author

Hi @bogdandrutu and @jmacd , this is only the skeleton for a processor which converts delta to rate. Would you please take a look.

Aneurysm9
Aneurysm9 approved these changes Jul 19, 2021
View reviewed changes
processor/deltatorateprocessor/config.go Outdated Show resolved Hide resolved
processor/deltatorateprocessor/doc.go Outdated Show resolved Hide resolved
processor/deltatorateprocessor/factory.go Outdated Show resolved Hide resolved
Collector automation moved this from In progress to Reviewer approved Jul 19, 2021
@hossain-rayhan
Copy link
Contributor Author

hossain-rayhan commented Jul 19, 2021 •
edited
Loading

Hi @bogdandrutu , would you please have a look? Can we get this merged so that I can rebase the acutal logic PR and get that reviewed?

bogdandrutu
bogdandrutu reviewed Jul 21, 2021
View reviewed changes
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

In an ideal world, would this be part of a generic "metric transform processor" or the "generate metrics processor"?

@hossain-rayhan
Copy link
Contributor Author

In an ideal world, would this be part of a generic "metric transform processor" or the "generate metrics processor"?

Hi @bogdandrutu, I remember from our discussion (with you and @jmacd), we wanted to to make this an independent processor for the sake of decoupling features. I am not sure if I missed anything here.

Also, this changes the metric types for the same metrics. So, I am OK to put them in the metricstransformprocessor. Any suggestion from your side, @bogdandrutu ?

@bogdandrutu
Copy link
Member

Hi @bogdandrutu, I remember from our discussion (with you and @jmacd), we wanted to to make this an independent processor for the sake of decoupling features. I am not sure if I missed anything here.

Both @jmacd and I suggested to decouple this logic from delta calculation, but my question was different.

@hossain-rayhan
Copy link
Contributor Author

Hi @bogdandrutu, I remember from our discussion (with you and @jmacd), we wanted to to make this an independent processor for the sake of decoupling features. I am not sure if I missed anything here.

Both @jmacd and I suggested to decouple this logic from delta calculation, but my question was different.

Thanks. I would prefer to put them in the metric transform processor as it does not generate new metrics but changes the type of the metrics.

@bogdandrutu
Copy link
Member

@punya as a leader of the metricstransform processor, I need your input :)

@hossain-rayhan
Copy link
Contributor Author

@punya as a leader of the metricstransform processor, I need your input :)

@punya can you please add your opinion here?

@hossain-rayhan
Copy link
Contributor Author

@bogdandrutu and @punya I need a decision here. Please help to move forward with this. "

CC: @alolita

@hossain-rayhan hossain-rayhan mentioned this pull request Jul 27, 2021
Merged
@punya
Copy link
Member

punya commented Jul 27, 2021

@bogdandrutu and @hossain-rayhan , sorry for the slow response here. I suggest we merge this as a separate processor. There are multiple ongoing efforts related to consolidating processors, and it is easier to merge than to split apart.

@bogdandrutu
Copy link
Member

Please fix lint issues, cannot merge

@hossain-rayhan
Copy link
Contributor Author

Hi @bogdandrutu I rebased and I believe it passed the lint and unit test. Not sure why build-packages are on queue forever.

@bogdandrutu bogdandrutu merged commit cad760c into open-telemetry:main Jul 29, 2021
Collector automation moved this from Reviewer approved to Done Jul 29, 2021
mstumpfx pushed a commit to mstumpfx/opentelemetry-collector-contrib that referenced this pull request Aug 31, 2021
@hossain-rayhan @Aneurysm9
Add skeleton for new delta to rate processor (open-telemetry#4218)
1dd65b2 
* Add skeleton for delta to rate processor

Signed-off-by: Rayhan Hossain <[email protected]>

* Update processor/deltatorateprocessor/config.go

Co-authored-by: Anthony Mirabella <[email protected]>

* Update processor/deltatorateprocessor/doc.go

Co-authored-by: Anthony Mirabella <[email protected]>

* Update processor/deltatorateprocessor/factory.go

Co-authored-by: Anthony Mirabella <[email protected]>

Co-authored-by: Anthony Mirabella <[email protected]>
hex1848 pushed a commit to hex1848/opentelemetry-collector-contrib that referenced this pull request Jun 2, 2022
reduce test time for TestGRPCMaxRecvSize test (open-telemetry#4218)
d440d05
@mx-psi mx-psi mentioned this pull request Oct 21, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aneurysm9 Aneurysm9 Aneurysm9 approved these changes

@bogdandrutu bogdandrutu bogdandrutu approved these changes

@jmacd jmacd jmacd approved these changes

Assignees

@mx-psi mx-psi

Labels
None yet
Projects
No open projects
Collector
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@hossain-rayhan @bogdandrutu @punya @Aneurysm9 @jmacd @mx-psi