Skip to content
This repository was archived by the owner on Jul 31, 2023. It is now read-only.

Add log exporter.#1126

Merged
rghetia merged 5 commits intocensus-instrumentation:devfrom
rghetia:log_exporter
Apr 23, 2019
Merged

Add log exporter.#1126
rghetia merged 5 commits intocensus-instrumentation:devfrom
rghetia:log_exporter

Conversation

@rghetia
Copy link
Copy Markdown
Contributor

@rghetia rghetia commented Apr 23, 2019

No description provided.

@rghetia rghetia requested review from a team and rakyll as code owners April 23, 2019 07:13
@rghetia rghetia requested a review from songy23 April 23, 2019 07:28
// limitations under the License.

// Package exporter contains a log exporter that supports exporting
// OpenCensus metrics to a logging framework.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: metrics and spans.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done.

"go.opencensus.io/trace"
)

// LogExporter exports stats to log file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: stats and spans

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed.

// If it is nil then the metrics are logged on console
MetricsLogFile string

//TracesLogFile is path where exported span data are logged.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: space after //

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done.


func printMetricDescriptor(metric *metricdata.Metric) string {
d := metric.Descriptor
return fmt.Sprintf("name: %s, type: %s, unit: %s ",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Label keys?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I print it as k=v pair in printLabels.

}
}

// Start starts the metric exporter.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: metric and trace

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it doesn't start trace exporter. For trace it sill requires to register but I don't see why I cannot call registerExporter here. I'll make the change and update the comment.

return e.ir.Start()
}

// Stop stops the metric exporter.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ditto

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done.

e.ir, _ = metricexport.NewIntervalReader(&metricexport.Reader{}, e)
})
e.ir.ReportingInterval = e.o.ReportingInterval
return e.ir.Start()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you need to register this as a trace exporter too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll register as trace exporter here. see my response to prev comment.

Copy link
Copy Markdown
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

}
f, err := os.OpenFile(filepath, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0666)
if err != nil {
log.Fatalf("error opening file: %v", err)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: consider returning err instead of logging it.

@rghetia rghetia merged commit 9719748 into census-instrumentation:dev Apr 23, 2019
@rghetia rghetia deleted the log_exporter branch April 23, 2019 22:28
rghetia added a commit to rghetia/opencensus-go that referenced this pull request Apr 25, 2019
* Add log exporter.

* close log files when program terminates.

* split stats into multiple lines.

* fix review comments.

* one more comment.
rghetia added a commit that referenced this pull request Apr 25, 2019
* Add log exporter.

* close log files when program terminates.

* split stats into multiple lines.

* fix review comments.

* one more comment.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants