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

Add log exporter. #1126

Merged
merged 5 commits into from
Apr 23, 2019
Merged

Add log exporter. #1126

merged 5 commits into from
Apr 23, 2019

Conversation

rghetia
Copy link
Contributor

@rghetia rghetia commented Apr 23, 2019

No description provided.

@rghetia rghetia requested review from rakyll and a team 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
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
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
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
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
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
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
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
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
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
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
Contributor

Choose a reason for hiding this comment

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

Ditto

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