Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

metrics: OpenCensus-Go stats+view to Metrics converter #32

Merged
merged 1 commit into from
Dec 9, 2018
Merged

metrics: OpenCensus-Go stats+view to Metrics converter #32

merged 1 commit into from
Dec 9, 2018

Conversation

odeke-em
Copy link
Contributor

This is the first step of piecemeal updates to allow
OpenCensus-Go to add metrics exporting before
https://github.com/census-instrumentation/opencensus-go#956
is resolved, as that issue is going to take a long time and
a lot of work.

After that, a follow-up change will be to then implement the
metrics service that will enable this exporter to
be an OpenCensus-Go stats+view exporter, then transform
that data into metricspb.Metric

Updates #31

odeke-em added a commit that referenced this pull request Nov 26, 2018
@odeke-em
metrics: the exporter can now convert from OpenCensus-Go stats to met…
2f146a1 
…rics

We now have a fully functioning
    view.Exporter --> metricspb.Exporter
converter.

Depends on #32
Updates #31
@odeke-em
Copy link
Contributor Author

I've also uploaded the part 2 of it. Unfortunately Github can't base a PR off one that hasn't yet been merged otherwise it'd have been a great candidate for splitting up. Anyways please take a look at the two different commits.

@odeke-em odeke-em mentioned this pull request Nov 26, 2018
Closed
@basvanbeek basvanbeek mentioned this pull request Nov 27, 2018
Merged
odeke-em added a commit that referenced this pull request Nov 27, 2018
@odeke-em
metrics: the exporter can now convert from OpenCensus-Go stats to met…
c5f1f46 
…rics

We now have a fully functioning
    view.Exporter --> metricspb.Exporter
converter.

Depends on #32
Updates #31
odeke-em added a commit that referenced this pull request Nov 27, 2018
@odeke-em
metrics: the exporter can now convert from OpenCensus-Go stats to met…
a8a9cff 
…rics

We now have a fully functioning
    view.Exporter --> metricspb.Exporter
converter.

Depends on #32
Updates #31
@odeke-em
Copy link
Contributor Author

Kindly paging @bogdandrutu @songy23 @rakyll, this issue is blocking the OpenCensus-PHP stats implementation, so please help me review it, thanks!

@odeke-em odeke-em requested a review from rakyll November 30, 2018 06:49
bogdandrutu
bogdandrutu previously requested changes Nov 30, 2018
View reviewed changes
Copy link
Contributor

@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.

This PR looks good on the high level. Some suggestions:

  1. Can you split it into a smaller PR than can be reviewed easily?
  2. In the same PR you include changes in the mod/sum - please move them in a separate PR.
  3. Ask a person with a good golang skill to review the language specific things.

@odeke-em
Copy link
Contributor Author

Thanks for the comments @bogdandrutu!

  1. Can you split it into a smaller PR than can be reviewed easily?

Sure, I can do that

  1. In the same PR you include changes in the mod/sum - please move them in a separate PR.

That go mod/sum change will have to be merged in before this otherwise CI won't build

  1. Ask a person with a good golang skill to review the language specific things.

Yes, I requested @rakyll and @songy23 for reviews. There aren't too many people working on OpenCensus-Go unfortunately.

odeke-em added a commit that referenced this pull request Nov 30, 2018
@odeke-em
go.{mod, sum}: preparing for Metrics service
48c455c 
As requested in #32, splitting this up to prepare
for the larger change. However this change needs
to be merged in first before any Metrics related one.
@odeke-em odeke-em mentioned this pull request Nov 30, 2018
Merged
@odeke-em
Copy link
Contributor Author

@bogdandrutu I've mailed #33 for go.mod and go.sum changes.

odeke-em added a commit that referenced this pull request Nov 30, 2018
@odeke-em
metrics: the exporter can now convert from OpenCensus-Go stats to met…
8cd2c5e 
…rics

We now have a fully functioning
    view.Exporter --> metricspb.Exporter
converter.

Depends on #32
Updates #31
odeke-em added a commit that referenced this pull request Nov 30, 2018
@odeke-em
Adapter to transform view.Data to metricsproto.Metric
2d312c2 
Code and tests to convert from OpenCensus-Go view.Data
to metricsproto.Metric. This will then allow us to
hook ocagent-exporter into OpenCensus-Go consuming
programs that export view.Data.

Updates #31
Cut from #32
@odeke-em odeke-em mentioned this pull request Nov 30, 2018
Merged
odeke-em added a commit that referenced this pull request Dec 9, 2018
@odeke-em
metrics: the exporter can now convert from OpenCensus-Go stats to met…
1213671 
…rics

We now have a fully functioning
    view.Exporter --> metricspb.Exporter
converter.

Depends on #32
Updates #31
@odeke-em
Copy link
Contributor Author

odeke-em commented Dec 9, 2018

Alright, since #32 has been merged into master and then rebased in this PR, what remains is just the basic uploading logic. PTAL @bogdandrutu @songy23

@odeke-em odeke-em dismissed bogdandrutu’s stale review December 9, 2018 00:21

Addressed splitting up PRs, thank you.

@odeke-em
metrics: the exporter can now convert from OpenCensus-Go stats to met…
59ea6a2 
…rics

We now have a fully functioning
    view.Exporter --> metricspb.Exporter
converter.

Depends on #32
Updates #31
songy23
songy23 approved these changes Dec 9, 2018
View reviewed changes
Copy link
Contributor

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

LGTM overall. Consider filing issues for adding resource support since it's needed in multiple places.

nodeinfo.go
// Pid from the current process
// StartTimestamp from the start time of this process
// Language and library information.
func NodeWithStartTime(nodeName string) *commonpb.Node {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would say I prefer the old method name :) Not only start_time is derived from env vars.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha. The reason why I changed it is because all the other variables can be retrieved alright from the environment but we don't expose startTime so it is the most important one.

@odeke-em
Copy link
Contributor Author

odeke-em commented Dec 9, 2018

Thank you @songy23 for the review!

LGTM overall. Consider filing issues for adding resource support since it's needed in multiple places.

In deed, I was actually just working on the resource inclusion but it has to come after this PR since we'll create a resource once when the exporter is started and then update each metric that's produced instead of creating a resource for every metric.

@odeke-em odeke-em merged commit 59ea6a2 into master Dec 9, 2018
@odeke-em
Copy link
Contributor Author

odeke-em commented Dec 9, 2018

@songy23 I've mailed out PR #35 which adds resources to initial trace and metrics message and after that we'll be gucci!

@odeke-em odeke-em deleted the stats-to-metrics-converter branch December 9, 2018 01:43
This was referenced Dec 9, 2018
Closed
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@songy23 songy23 songy23 approved these changes

@bogdandrutu bogdandrutu bogdandrutu left review comments

@rakyll rakyll Awaiting requested review from rakyll

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@odeke-em @bogdandrutu @songy23 @googlebot