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

Add tags package based on the specs. #172

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add tags package based on the specs. #172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
85fd464
Add tags package based on the specs.
bogdandrutu Jan 16, 2019
5d2823d
Update comments and View definition to use TagKey
bogdandrutu Jan 17, 2019
879ea3a
Make TagScope enum and add comment about the scope not being exported.
bogdandrutu Jan 18, 2019
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
4 changes: 3 additions & 1 deletion src/opencensus/proto/stats/v1/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ proto_library(
name = "stats_proto",
srcs = ["stats.proto"],
deps = [
"//opencensus/proto/tags/v1:tags_proto",
"@com_google_protobuf//:duration_proto",
"@com_google_protobuf//:timestamp_proto",
],
Expand All @@ -37,9 +38,10 @@ java_proto_library(

go_proto_library(
name = "stats_proto_go",
proto = ":stats_proto",
importpath = "github.com/census-instrumentation/opencensus-proto/gen-go/stats/v1",
proto = ":stats_proto",
deps = [
"//opencensus/proto/tags/v1:tags_proto_go",
"@com_github_golang_protobuf//ptypes/timestamp:go_default_library",
],
)
12 changes: 3 additions & 9 deletions src/opencensus/proto/stats/v1/stats.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,20 +17,14 @@ syntax = "proto3";
package opencensus.proto.stats.v1;

import "google/protobuf/timestamp.proto";
import "opencensus/proto/tags/v1/tags.proto";

option go_package = "github.com/census-instrumentation/opencensus-proto/gen-go/stats/v1";

option java_multiple_files = true;
option java_package = "io.opencensus.proto.stats.v1";
option java_outer_classname = "StatsProto";

// TODO(bdrutu): Consider if this should be moved to a "tags" directory to match the API structure.
message Tag {
string key = 1;
string value = 2;
}

// Measure .
message Measure {
// A string by which the measure will be referred to, e.g. "rpc_server_latency". Names MUST be
// unique within the library.
Expand Down Expand Up @@ -70,7 +64,7 @@ message View {
// An array of tag keys. These values associated with tags of this name form the basis by which
// individual stats will be aggregated (one aggregation per unique tag value). If none are
// provided, then all data is recorded in a single aggregation.
repeated string columns = 4;
repeated opencensus.proto.tags.v1.TagKey columns = 4;

// The description of the aggregation used for this view which describes how data collected are
// aggregated.
Expand Down Expand Up @@ -119,7 +113,7 @@ message DistributionAggregation {

// Describes a data point to be collected for a Measure.
message Measurement {
repeated Tag tags = 1;
opencensus.proto.tags.v1.TagMap tag_map = 1;

// The name of the measure to which the value is applied.
string measure_name = 2;
Expand Down
41 changes: 41 additions & 0 deletions src/opencensus/proto/tags/v1/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# Copyright 2019, OpenCensus Authors
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

package(default_visibility = ["//visibility:public"])

load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library")

proto_library(
name = "tags_proto",
srcs = ["tags.proto"],
)

cc_proto_library(
name = "tags_proto_cc",
deps = [":tags_proto"],
)

java_proto_library(
name = "tags_proto_java",
deps = [":tags_proto"],
)

go_proto_library(
name = "tags_proto_go",
importpath = "github.com/census-instrumentation/opencensus-proto/gen-go/tags/v1",
proto = ":tags_proto",
deps = [
"@com_github_golang_protobuf//ptypes/timestamp:go_default_library",
],
)
82 changes: 82 additions & 0 deletions src/opencensus/proto/tags/v1/tags.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
// Copyright 2019, OpenCensus Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

syntax = "proto3";

package opencensus.proto.tags.v1;

option go_package = "github.com/census-instrumentation/opencensus-proto/gen-go/tags/v1";

option java_multiple_files = true;
option java_package = "io.opencensus.proto.tags.v1";
option java_outer_classname = "TagsProto";

// TagScope is used to determine the scope of a Tag.
//
//
enum TagScope {
// Unknown type.
TYPE_UNSPECIFIED = 0;
// Tag with Local scope are used within the process it created.
LOCAL_SCOPE = 1;
// A tag is created with the Request scope then it is propagated across
// process boundaries subject to outgoing and incoming (on remote side)
// filter criteria.
REQUEST_SCOPE = 2;
}

// TagKey is the name of the Tag.
message TagKey {
// Restrictions:
// * MUST contain only printable ASCII (codes between 32 and 126 inclusive)
// * MUST have length greater than zero and less than 256.
// * MUST not be empty.
//
// This field is required.
string value = 1;
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be like -> string name = 1;

}

// TagValue is the value associated with a Tag.
message TagValue {
// It MUST contain only printable ASCII (codes between 32 and 126)
//
Copy link
Member

Choose a reason for hiding this comment

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

It cannot be longer than 256 right? https://github.com/census-instrumentation/opencensus-java/blob/master/api/src/main/java/io/opencensus/tags/TagValue.java#L49

// This field is required.
string value = 1;
Copy link
Member

Choose a reason for hiding this comment

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

why not truncatable string? If it is not a truncatable string - what's the reason to have a message for it and not simply use string as before? Same comment for TagKey

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason to have a message:

  1. For the tag key in the future we may add other things - e.g. type information (string, int, etc.) or any other metadata.
  2. For the tag value in the future we may support other types like int.

Copy link
Member

Choose a reason for hiding this comment

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

I got why we need a message for TagValue. This comment is more about the fact that there is no consistency in current proto definitions on whether string or TruncatableString is used. Or I am missing something. AttributeValue and Span name uses TruncatableString whereas Status and Tracestate uses string. So my question is what's the deciding factor here of whether to use one or another

}

// A Tag consists of TagScope, TagKey, and TagValue.
message Tag {
// This field is required.
TagKey key = 1;

// This field is required.
TagValue value = 2;

// This field is optional, if not present the default scope is REQUEST_SCOPE
// (the motivation is backwards compatibility).
//
// This field is only needed for data generation and tags defining cases, not
// for exporting.
TagScope scope = 3;
}

// TagMap is an abstract data type that represents collection of tags. i.e.,
// each key is associated with exactly one value.
message TagMap {
// The list of tags MUST contains unique TagKeys.
//
// Not a map becuase the proto3 map does not allow a message as the key.
// https://developers.google.com/protocol-buffers/docs/proto3#maps
repeated Tag tags = 1;
Copy link
Member

Choose a reason for hiding this comment

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

so tags with the same name is allowed, correct? Perhaps something should be said about it in comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The list should contains unique TagKeys. The proto3 map does not allow the map_key to be a proto message see https://developers.google.com/protocol-buffers/docs/proto3#maps.

Copy link
Member

Choose a reason for hiding this comment

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

The fact that the same key value is not allowed is not clear from the comment at all. As minimum I suggest to indicate this requirement in a comment.

Copy link
Member

Choose a reason for hiding this comment

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

From my perspective - compromising on data integrity by not forcing uniqueness of keys in favor of potential extensibility sounds a hard sell in this case. Do you see a scenario in which key will be typed? Any other information like PII flag may be moved to the value. Or I am missing something?

Copy link
Member

Choose a reason for hiding this comment

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

BTW, map with the string key is used for Attributes already. Arguably Attributes keys should have all the same scenarios as tags.

Copy link
Member

Choose a reason for hiding this comment

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

Just to clarify - some misunderstanding may be coming from how you can read the sentence "each key is associated with exactly one value.". If you treat "each key" as unique string - it is already handled. But I read it as one key object maps to one value object. I see you already handled it, just clarifying what I meant,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason of using a TagKey instead of directly string is because few reasons:

  1. Later we can make the TagKey { oneof (string, int) } where int may represent a global id if you have a global registry for all the tags.
  2. For the string in TagKey we need to do validation (limited char set) so this way people can create the key once and avoid validation every time in the api.

}