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

fix(metrics): ostream exporter should print out resource attributes #1523

Merged
merged 9 commits into from
Aug 5, 2022
Merged

fix(metrics): ostream exporter should print out resource attributes #1523

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6a518dc
fix(metrics): ostream exporter should print out resource attributes
bsarden Jul 28, 2022
1eec14e
test(exporters): update ostream_metric_test to check for resource attrs
bsarden Jul 28, 2022
79124bb
chore: fix lint
bsarden Jul 29, 2022
361c13f
fix(test): update ostream_metrics_test to new opentelemetry sdk version
bsarden Jul 31, 2022
ade6377
test(ostream_metrics): use version.h instead of hardcoding sdk version
bsarden Jul 31, 2022
a56d235
Merge branch 'main' into dev/metrics-resources-ostream-exporter
lalitb Aug 1, 2022
af0ee1f
Merge branch 'main' into dev/metrics-resources-ostream-exporter
lalitb Aug 2, 2022
05489f8
feat(metrics): enforce iteration order when printing resource attrs
bsarden Aug 5, 2022
672354b
Merge branch 'main' into dev/metrics-resources-ostream-exporter
lalitb Aug 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion exporters/ostream/include/opentelemetry/exporters/ostream/metric_exporter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,13 @@ class OStreamMetricExporter final : public opentelemetry::sdk::metrics::MetricEx
bool is_shutdown_ = false;
mutable opentelemetry::common::SpinLockMutex lock_;
bool isShutdown() const noexcept;
void printInstrumentationInfoMetricData(const sdk::metrics::ScopeMetrics &info_metrics);
void printInstrumentationInfoMetricData(const sdk::metrics::ScopeMetrics &info_metrics,
const sdk::metrics::ResourceMetrics &data);
void printPointData(const opentelemetry::sdk::metrics::PointType &point_data);
void printPointAttributes(const opentelemetry::sdk::metrics::PointAttributes &point_attributes);
void printAttributes(const std::unordered_map<std::string, sdk::common::OwnedAttributeValue> &map,
const std::string prefix);
void printResources(const opentelemetry::sdk::resource::Resource &resources);
};
} // namespace metrics
} // namespace exporter
Expand Down
28 changes: 26 additions & 2 deletions exporters/ostream/src/metric_exporter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,34 @@ sdk::common::ExportResult OStreamMetricExporter::Export(

for (auto &record : data.scope_metric_data_)
{
printInstrumentationInfoMetricData(record);
printInstrumentationInfoMetricData(record, data);
lalitb marked this conversation as resolved.
Show resolved Hide resolved
}
return sdk::common::ExportResult::kSuccess;
}

void OStreamMetricExporter::printAttributes(
const std::unordered_map<std::string, sdk::common::OwnedAttributeValue> &map,
const std::string prefix)
{
for (const auto &kv : map)
Copy link
Member

@lalitb lalitb Aug 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@bsarden - Looks like this part of the code is causing problem. As it's unordered_map, the order of printing of resource elements is implementation specific (so different in Windows and Linux). Can we ensure to print them in the given order - maybe move resource data to std::map first and then print, and then also ensure that the same order is used for comparison in unit-test? Or else do some smart comparison for resource data in unit-test.

Copy link
Member

Choose a reason for hiding this comment

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

@bsarden - Just wondering if you have a plan to fix CI as mentioned above? Else, I can do it on top of your repo if you are fine :)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for fixing it. CI looks good now :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the ping and reproducing the error on a windows box!

{
sout_ << prefix << kv.first << ": ";
opentelemetry::exporter::ostream_common::print_value(kv.second, sout_);
}
}

void OStreamMetricExporter::printResources(const opentelemetry::sdk::resource::Resource &resources)
{
auto attributes = resources.GetAttributes();
if (attributes.size())
{
printAttributes(attributes, "\n\t");
}
}

void OStreamMetricExporter::printInstrumentationInfoMetricData(
const sdk::metrics::ScopeMetrics &info_metric)
const sdk::metrics::ScopeMetrics &info_metric,
const sdk::metrics::ResourceMetrics &data)
{
// sout_ is shared
const std::lock_guard<opentelemetry::common::SpinLockMutex> locked(lock_);
Expand All @@ -109,6 +130,9 @@ void OStreamMetricExporter::printInstrumentationInfoMetricData(
printPointAttributes(pd.attributes);
}
}

sout_ << "\n resources\t:";
printResources(*data.resource_);
}
sout_ << "\n}\n";
}
Expand Down
20 changes: 20 additions & 0 deletions exporters/ostream/test/ostream_metric_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,11 @@ TEST(OStreamMetricsExporter, ExportSumPointData)
"\n value\t\t: 20"
"\n attributes\t\t: "
"\n\ta1: b1"
"\n resources\t:"
"\n\tservice.name: unknown_service"
"\n\ttelemetry.sdk.version: 1.5.0"
"\n\ttelemetry.sdk.name: opentelemetry"
"\n\ttelemetry.sdk.language: cpp"
"\n}\n";
ASSERT_EQ(stdoutOutput.str(), expected_output);
}
Expand Down Expand Up @@ -151,6 +156,11 @@ TEST(OStreamMetricsExporter, ExportHistogramPointData)
"\n counts : [200, 300, 400, 500, ]"
"\n attributes\t\t: "
"\n\ta1: b1"
"\n resources\t:"
"\n\tservice.name: unknown_service"
"\n\ttelemetry.sdk.version: 1.5.0"
Copy link
Member

Choose a reason for hiding this comment

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

Can we use the macro OPENTELEMETRY_SDK_VERSION instead of the hardcoding version? This way, we don't need to modify this part of the code with every new release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Done. There are some strange cmake failures that I'm having difficulties reproducing on my ubuntu 20.04 machine locally, but they are also not marked as "required" on the CI stages...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lalitb: What are the next steps I need to take here in order to debug? It's not clear to me what the next debugging steps are (or how some of these CI failures are relevant to a largely NFC patch like this one).

"\n\ttelemetry.sdk.name: opentelemetry"
"\n\ttelemetry.sdk.language: cpp"
"\n}\n";
ASSERT_EQ(stdoutOutput.str(), expected_output);
}
Expand Down Expand Up @@ -214,6 +224,11 @@ TEST(OStreamMetricsExporter, ExportLastValuePointData)
"\n valid : true"
"\n value : 20"
"\n attributes\t\t: "
"\n resources\t:"
"\n\tservice.name: unknown_service"
"\n\ttelemetry.sdk.version: 1.5.0"
"\n\ttelemetry.sdk.name: opentelemetry"
"\n\ttelemetry.sdk.language: cpp"
"\n}\n";
ASSERT_EQ(stdoutOutput.str(), expected_output);
}
Expand Down Expand Up @@ -261,6 +276,11 @@ TEST(OStreamMetricsExporter, ExportDropPointData)
"\n name\t\t: library_name"
"\n description\t: description"
"\n unit\t\t: unit"
"\n resources\t:"
"\n\tservice.name: unknown_service"
"\n\ttelemetry.sdk.version: 1.5.0"
"\n\ttelemetry.sdk.name: opentelemetry"
"\n\ttelemetry.sdk.language: cpp"
"\n}\n";

ASSERT_EQ(stdoutOutput.str(), expected_output);
Expand Down