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
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
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",
],
)
10 changes: 2 additions & 8 deletions src/opencensus/proto/stats/v1/stats.proto
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 @@ -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
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",
],
)
79 changes: 79 additions & 0 deletions src/opencensus/proto/tags/v1/tags.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
// 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.
message TagScope {
enum Scope {
bogdandrutu marked this conversation as resolved.
Show resolved Hide resolved
// 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;
}

// This field is required.
Scope scope = 1;
}

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

// 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).
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 {
// Not a map becuase the proto3 map does not allow a message as the key.
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.

}