Skip to content

Conversation

@pmm-sumo
Copy link
Contributor

@pmm-sumo pmm-sumo commented Sep 26, 2021

Testing: Unit tests updated, manual tests to follow

@codecov
Copy link

codecov bot commented Sep 29, 2021

Codecov Report

Merging #4117 (d9c27cc) into main (6edb503) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4117   +/-   ##
=======================================
  Coverage   87.89%   87.90%           
=======================================
  Files         174      175    +1     
  Lines       10198    10200    +2     
=======================================
+ Hits         8964     8966    +2     
  Misses        984      984           
  Partials      250      250           
Impacted Files Coverage Δ
exporter/exporterhelper/factory.go 100.00% <100.00%> (ø)
...er/exporterhelper/internal/observability_stable.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6edb503...d9c27cc. Read the comment docs.

@pmm-sumo pmm-sumo force-pushed the pq-move-to-internal-part3 branch from baf5330 to d9c27cc Compare September 29, 2021 16:28
@pmm-sumo pmm-sumo marked this pull request as ready for review September 29, 2021 16:29
@pmm-sumo pmm-sumo requested review from a team and tigrannajaryan September 29, 2021 16:29
@pmm-sumo
Copy link
Contributor Author

@tigrannajaryan thank you for merging the part 2. Here comes the last of the series - I thought that it would be good to expose some metrics on how persistent queue is doing

@bogdandrutu bogdandrutu changed the title Move Persistent Queue to exporthelper/internal [Part 3/3] Including metrics on number of batches handled through persistent queue Sep 30, 2021
Copy link
Member

@tigrannajaryan tigrannajaryan 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 one question.

zap.String(zapQueueNameKey, pcs.queueName), zap.Error(err))
}

recordCurrentlyDispatchedBatches(ctx, len(pcs.currentlyDispatchedItems), pcs.queueName)
Copy link
Member

Choose a reason for hiding this comment

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

Do we only record these new metrics for persistent queue or also for in-memory queue when persistent queue is disabled?
I think they are useful regardless of whether persistence is enabled or no.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was wondering about doing that for in-memory queue as well though it seems it's using the old API and metrics would need to be pretty much redone there

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.

@tigrannajaryan I am not sold on this, first we should stop adding "random" metrics (we need a plan of how to name metrics, what is important vs not important to monitor, etc.) because sooner than later collector's own telemetry will need to stabilize, and so far I am the only one dealing with this work.

@pmm-sumo
Copy link
Contributor Author

pmm-sumo commented Oct 1, 2021

I am not sold on this, first we should stop adding "random" metrics (we need a plan of how to name metrics, what is important vs not important to monitor, etc.) because sooner than later collector's own telemetry will need to stabilize, and so far I am the only one dealing with this work.

That's a fair argument. I think queued_retry might use redoing metrics. If you have some suggestion @bogdandrutu how would you like to see them organised I will be happy to take care of that.

BTW, I was thinking about adding some more metrics in the future to persistent queue which would hold info on number of operations underneath

@tigrannajaryan
Copy link
Member

@tigrannajaryan I am not sold on this, first we should stop adding "random" metrics (we need a plan of how to name metrics, what is important vs not important to monitor, etc.) because sooner than later collector's own telemetry will need to stabilize, and so far I am the only one dealing with this work.

Sounds good. Perhaps that can also eventually feed into a broader scope of defining semantic conventions for "data processors" in Otel spec, not just the Collector.

@pmm-sumo
Copy link
Contributor Author

pmm-sumo commented Oct 4, 2021

Considering the discussion above and idea of leveraging a better strategy for collector metrics overall, I am closing this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants