[GitHub] [skywalking-banyandb] lujiajing1126 commented on a diff in pull request #239: Unify TagFilter for Streaming scenario
lujiajing1126 commented on code in PR #239:
URL:
https://github.com/apache/skywalking-banyandb/pull/239#discussion_r1082603413
##
banyand/measure/measure_write.go:
##
@@ -39,17 +39,18 @@ import (
var errMalformedElement = errors.New("element is malformed")
-func (s *measure) write(md *commonv1.Metadata, shardID common.ShardID, entity
[]byte, entityValues tsdb.EntityValues, value *measurev1.DataPointValue) error {
+func (s *measure) write(sm *databasev1.Measure, shardID common.ShardID, entity
[]byte, entityValues tsdb.EntityValues,
Review Comment:
Removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on a diff in pull request #239: Unify TagFilter for Streaming scenario
lujiajing1126 commented on code in PR #239:
URL:
https://github.com/apache/skywalking-banyandb/pull/239#discussion_r1082600211
##
banyand/liaison/grpc/registry.go:
##
@@ -435,7 +437,37 @@ type topNAggregationRegistryServer struct {
func (ts *topNAggregationRegistryServer) Create(ctx context.Context,
req *databasev1.TopNAggregationRegistryServiceCreateRequest,
) (*databasev1.TopNAggregationRegistryServiceCreateResponse, error) {
- if err :=
ts.schemaRegistry.TopNAggregationRegistry().CreateTopNAggregation(ctx,
req.GetTopNAggregation()); err != nil {
+ topNSchema := req.GetTopNAggregation()
Review Comment:
I think we should avoid creating new measures while loading...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on a diff in pull request #239: Unify TagFilter for Streaming scenario
lujiajing1126 commented on code in PR #239:
URL:
https://github.com/apache/skywalking-banyandb/pull/239#discussion_r1082598524
##
banyand/liaison/grpc/registry.go:
##
@@ -435,7 +437,37 @@ type topNAggregationRegistryServer struct {
func (ts *topNAggregationRegistryServer) Create(ctx context.Context,
req *databasev1.TopNAggregationRegistryServiceCreateRequest,
) (*databasev1.TopNAggregationRegistryServiceCreateResponse, error) {
- if err :=
ts.schemaRegistry.TopNAggregationRegistry().CreateTopNAggregation(ctx,
req.GetTopNAggregation()); err != nil {
+ topNSchema := req.GetTopNAggregation()
Review Comment:
We cannot do this since `topNProcessorManager` will start in the `PreRun`
stage, but the measure notifier is registered in the `Serve` stage which will
not be called if new measures are create.Any idea?
##
banyand/liaison/grpc/registry.go:
##
@@ -435,7 +437,37 @@ type topNAggregationRegistryServer struct {
func (ts *topNAggregationRegistryServer) Create(ctx context.Context,
req *databasev1.TopNAggregationRegistryServiceCreateRequest,
) (*databasev1.TopNAggregationRegistryServiceCreateResponse, error) {
- if err :=
ts.schemaRegistry.TopNAggregationRegistry().CreateTopNAggregation(ctx,
req.GetTopNAggregation()); err != nil {
+ topNSchema := req.GetTopNAggregation()
Review Comment:
We cannot do this since `topNProcessorManager` will start in the `PreRun`
stage, but the measure notifier is registered in the `Serve` stage which will
not be called if new measures are create.Any idea? @hanahmily
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on a diff in pull request #239: Unify TagFilter for Streaming scenario
lujiajing1126 commented on code in PR #239:
URL:
https://github.com/apache/skywalking-banyandb/pull/239#discussion_r1082550935
##
banyand/liaison/grpc/registry.go:
##
@@ -435,7 +437,37 @@ type topNAggregationRegistryServer struct {
func (ts *topNAggregationRegistryServer) Create(ctx context.Context,
req *databasev1.TopNAggregationRegistryServiceCreateRequest,
) (*databasev1.TopNAggregationRegistryServiceCreateResponse, error) {
- if err :=
ts.schemaRegistry.TopNAggregationRegistry().CreateTopNAggregation(ctx,
req.GetTopNAggregation()); err != nil {
+ topNSchema := req.GetTopNAggregation()
Review Comment:
This part is moved to `measure` pkg. PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking-rover] mrproliu merged pull request #73: Support OpenSSL 3.0.x
mrproliu merged PR #73: URL: https://github.com/apache/skywalking-rover/pull/73 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on a diff in pull request #239: Unify TagFilter for Streaming scenario
lujiajing1126 commented on code in PR #239:
URL:
https://github.com/apache/skywalking-banyandb/pull/239#discussion_r1082551208
##
banyand/liaison/grpc/registry.go:
##
@@ -435,7 +437,37 @@ type topNAggregationRegistryServer struct {
func (ts *topNAggregationRegistryServer) Create(ctx context.Context,
req *databasev1.TopNAggregationRegistryServiceCreateRequest,
) (*databasev1.TopNAggregationRegistryServiceCreateResponse, error) {
- if err :=
ts.schemaRegistry.TopNAggregationRegistry().CreateTopNAggregation(ctx,
req.GetTopNAggregation()); err != nil {
+ topNSchema := req.GetTopNAggregation()
+ if err :=
ts.schemaRegistry.TopNAggregationRegistry().CreateTopNAggregation(ctx,
topNSchema); err != nil {
+ return nil, err
+ }
+
+ sourceMeasure, err :=
ts.schemaRegistry.MeasureRegistry().GetMeasure(ctx,
topNSchema.GetSourceMeasure())
+ if err != nil {
+ return nil, err
+ }
+ // create a "virtual" measure for TopN result
Review Comment:
Use `derived` now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on a diff in pull request #239: Unify TagFilter for Streaming scenario
lujiajing1126 commented on code in PR #239:
URL:
https://github.com/apache/skywalking-banyandb/pull/239#discussion_r1082550935
##
banyand/liaison/grpc/registry.go:
##
@@ -435,7 +437,37 @@ type topNAggregationRegistryServer struct {
func (ts *topNAggregationRegistryServer) Create(ctx context.Context,
req *databasev1.TopNAggregationRegistryServiceCreateRequest,
) (*databasev1.TopNAggregationRegistryServiceCreateResponse, error) {
- if err :=
ts.schemaRegistry.TopNAggregationRegistry().CreateTopNAggregation(ctx,
req.GetTopNAggregation()); err != nil {
+ topNSchema := req.GetTopNAggregation()
Review Comment:
This part is moved to `measure` pkg. PTAL
##
banyand/liaison/grpc/registry.go:
##
@@ -453,12 +485,13 @@ func (ts *topNAggregationRegistryServer) Update(ctx
context.Context,
func (ts *topNAggregationRegistryServer) Delete(ctx context.Context,
req *databasev1.TopNAggregationRegistryServiceDeleteRequest,
) (*databasev1.TopNAggregationRegistryServiceDeleteResponse, error) {
- ok, err :=
ts.schemaRegistry.TopNAggregationRegistry().DeleteTopNAggregation(ctx,
req.GetMetadata())
- if err != nil {
+ okTopNDeleted, errDelTopN :=
ts.schemaRegistry.TopNAggregationRegistry().DeleteTopNAggregation(ctx,
req.GetMetadata())
+ okMeasureDeleted, errDelMeasure :=
ts.schemaRegistry.MeasureRegistry().DeleteMeasure(ctx, req.GetMetadata())
Review Comment:
Removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on a diff in pull request #239: Unify TagFilter for Streaming scenario
lujiajing1126 commented on code in PR #239:
URL:
https://github.com/apache/skywalking-banyandb/pull/239#discussion_r1082550614
##
banyand/liaison/grpc/registry.go:
##
@@ -435,7 +437,37 @@ type topNAggregationRegistryServer struct {
func (ts *topNAggregationRegistryServer) Create(ctx context.Context,
req *databasev1.TopNAggregationRegistryServiceCreateRequest,
) (*databasev1.TopNAggregationRegistryServiceCreateResponse, error) {
- if err :=
ts.schemaRegistry.TopNAggregationRegistry().CreateTopNAggregation(ctx,
req.GetTopNAggregation()); err != nil {
+ topNSchema := req.GetTopNAggregation()
+ if err :=
ts.schemaRegistry.TopNAggregationRegistry().CreateTopNAggregation(ctx,
topNSchema); err != nil {
+ return nil, err
+ }
+
+ sourceMeasure, err :=
ts.schemaRegistry.MeasureRegistry().GetMeasure(ctx,
topNSchema.GetSourceMeasure())
+ if err != nil {
+ return nil, err
+ }
+ // create a "virtual" measure for TopN result
+ topNMeasure := &databasev1.Measure{
+ Metadata: topNSchema.GetMetadata(),
+ Interval: sourceMeasure.GetInterval(),
+ TagFamilies: []*databasev1.TagFamilySpec{
+ {
+ Name: measure.TopNTagFamily,
+ Tags: []*databasev1.TagSpec{
+ {
+ Name: "measureID",
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking] wu-sheng merged pull request #10296: Enhance OAP HTTP server to support HTTPS
wu-sheng merged PR #10296: URL: https://github.com/apache/skywalking/pull/10296 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #9883: [Feature] Support AWS service monitoring
wu-sheng commented on issue #9883: URL: https://github.com/apache/skywalking/issues/9883#issuecomment-1398364455 > Maybe this helps. https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/util/VarInt.java#L88 > Notice, we can't copy codes. Can't, I meam, if we need to copy, we need to update the license in the root about mentioning this copy. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #9883: [Feature] Support AWS service monitoring
wu-sheng commented on issue #9883: URL: https://github.com/apache/skywalking/issues/9883#issuecomment-1398354511 The process doc should be https://developers.google.com/protocol-buffers/docs/encoding#varints Maybe this helps. https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/util/VarInt.java#L88 Notice, we can't copy codes. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10296: Enhance OAP HTTP server to support HTTPS
wu-sheng commented on PR #10296: URL: https://github.com/apache/skywalking/pull/10296#issuecomment-1398330679 > > > > In another way, our gRPC service seems to have the same issue(can't update cert in the runtime), right? We would not call that not product-ready. > > > > > > > > > gRPC can reload the certs in about every 10 seconds. > > > > > > How is `armeria`? Could it do that? If so, polish is good. If it still can't, I feel still fine. > > It’s not built in for gRPC, it’s skywalking’s implementation does the reloading work. Armeria doesn’t do that out of the box either. Feel free to merge Got it. Saw `AbstractSslContext`. Yes, it is SkyWalking's implementation. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-rover] wu-sheng commented on pull request #73: Support OpenSSL 3.0.x
wu-sheng commented on PR #73: URL: https://github.com/apache/skywalking-rover/pull/73#issuecomment-1398324164 I updated the PR title and change log to avoid the `intentional`-sytle word. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-rover] spacewander commented on pull request #73: Try to support OpenSSL 3.0.x
spacewander commented on PR #73: URL: https://github.com/apache/skywalking-rover/pull/73#issuecomment-1398279687 > Thanks for your contribution to support OpenSSL `3.0.x`. Could you also update the change log? Done -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #10296: Enhance OAP HTTP server to support HTTPS
kezhenxu94 commented on PR #10296: URL: https://github.com/apache/skywalking/pull/10296#issuecomment-1398246733 > > > In another way, our gRPC service seems to have the same issue(can't update cert in the runtime), right? We would not call that not product-ready. > > > > > > gRPC can reload the certs in about every 10 seconds. > > How is `armeria`? Could it do that? If so, polish is good. If it still can't, I feel still fine. It’s not built in for gRPC, it’s skywalking’s implementation does the reloading work. Armeria doesn’t do that either out of the box either. Feel free to merge -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] pg-yang commented on issue #9883: [Feature] Support AWS service monitoring
pg-yang commented on issue #9883: URL: https://github.com/apache/skywalking/issues/9883#issuecomment-1398242330 OK, vary int 32. 🔢 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] pg-yang commented on issue #9883: [Feature] Support AWS service monitoring
pg-yang commented on issue #9883: URL: https://github.com/apache/skywalking/issues/9883#issuecomment-1398226560 ```java final byte[] decode = Base64.getDecoder().decode(s); final byte[] decode2 = Base64.getDecoder().decode(s2); final ResourceMetrics resourceMetrics = ResourceMetrics.parseFrom(Arrays.copyOfRange(decode, 5, decode.length)); final ResourceMetrics resourceMetrics2 = ResourceMetrics.parseFrom(Arrays.copyOfRange(decode2, 5, decode2.length)); ``` Hello, above code could work, but I don't know why. s and s2 encoded by base64. As mentioned in [OpenTelemetry 0.7.0 format](https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch-metric-streams-formats-opentelemetry.html): Each data structure starts with a header with an UnsignedVarInt32 indicating the record length in bytes. `UnsignedVarInt32` is 4 bytes, right? But the code works correctly when I skip 5 byte, skipping 4 byte will encounter an exception. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-banyandb] hanahmily commented on a diff in pull request #241: fix metadata register bug.
hanahmily commented on code in PR #241:
URL:
https://github.com/apache/skywalking-banyandb/pull/241#discussion_r1082356373
##
banyand/metadata/schema/group.go:
##
@@ -87,6 +88,9 @@ func (e *etcdSchemaRegistry) DeleteGroup(ctx context.Context,
group string) (boo
}
func (e *etcdSchemaRegistry) CreateGroup(ctx context.Context, group
*commonv1.Group) error {
+ if group.UpdatedAt != nil {
Review Comment:
Reflection? Could you elaborate more on this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking-banyandb] hanahmily commented on a diff in pull request #241: fix metadata register bug.
hanahmily commented on code in PR #241:
URL:
https://github.com/apache/skywalking-banyandb/pull/241#discussion_r1082356373
##
banyand/metadata/schema/group.go:
##
@@ -87,6 +88,9 @@ func (e *etcdSchemaRegistry) DeleteGroup(ctx context.Context,
group string) (boo
}
func (e *etcdSchemaRegistry) CreateGroup(ctx context.Context, group
*commonv1.Group) error {
+ if group.UpdatedAt != nil {
Review Comment:
Refection? Would you be able to elaborate more on this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking-banyandb] hanahmily commented on a diff in pull request #241: fix metadata register bug.
hanahmily commented on code in PR #241:
URL:
https://github.com/apache/skywalking-banyandb/pull/241#discussion_r1082346928
##
api/proto/banyandb/database/v1/schema.proto:
##
@@ -88,25 +89,25 @@ enum CompressionMethod {
// FieldSpec is the specification of field
message FieldSpec {
// name is the identity of a field
- string name = 1;
+ string name = 1 [(validate.rules).string.min_len = 1];
// field_type denotes the type of field value
- FieldType field_type = 2;
+ FieldType field_type = 2 [(validate.rules).enum.defined_only = true];
// encoding_method indicates how to encode data during writing
- EncodingMethod encoding_method = 3;
+ EncodingMethod encoding_method = 3 [(validate.rules).enum.defined_only =
true];
// compression_method indicates how to compress data during writing
- CompressionMethod compression_method = 4;
+ CompressionMethod compression_method = 4 [(validate.rules).enum.defined_only
= true];
}
// Measure intends to store data point
message Measure {
// metadata is the identity of a measure
- common.v1.Metadata metadata = 1;
+ common.v1.Metadata metadata = 1 [(validate.rules).message.required = true];
// tag_families are for filter measures
- repeated TagFamilySpec tag_families = 2;
+ repeated TagFamilySpec tag_families = 2 [(validate.rules).repeated.min_items
= 1];
// fields denote measure values
repeated FieldSpec fields = 3;
// entity indicates which tags will be to generate a series and shard a
measure
- Entity entity = 4;
+ Entity entity = 4 [(validate.rules).message.required = true];
// interval indicates how frequently to send a data point
// valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h", "d".
string interval = 5;
Review Comment:
RegExp might work, but I never like such expressions that are a bit tricky.
How about validating it in the metadata model by `timestamp.ParseDuration`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10298: feature: windows monitoring
wu-sheng commented on PR #10298: URL: https://github.com/apache/skywalking/pull/10298#issuecomment-1398149225 > If we cannot run the exporter on a Windows system, does it make sense to mock the data and run a test, what actually is tested? The MAL engine? We need to test MAL expressions. At least we did this for EKS monitoring last time. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10296: Enhance OAP HTTP server to support HTTPS
wu-sheng commented on PR #10296: URL: https://github.com/apache/skywalking/pull/10296#issuecomment-1398144197 > > In another way, our gRPC service seems to have the same issue(can't update cert in the runtime), right? We would not call that not product-ready. > > gRPC can reload the certs in about every 10 seconds. How is `armeria`? Could it do that? If so, polish is good. If it still can't, I feel still fine. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #10298: feature: windows monitoring
kezhenxu94 commented on PR #10298: URL: https://github.com/apache/skywalking/pull/10298#issuecomment-1398140149 If we cannot run the exporter on a Windows system, does it make sense to mock the data and run a test, what actually is tested? The MAL engine? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #10296: Enhance OAP HTTP server to support HTTPS
kezhenxu94 commented on PR #10296: URL: https://github.com/apache/skywalking/pull/10296#issuecomment-1398137826 > In another way, our gRPC service seems to have the same issue(can't update cert in the runtime), right? We would not call that not product-ready. gRPC can reload the certs in about every 10 seconds. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10296: Enhance OAP HTTP server to support HTTPS
wu-sheng commented on PR #10296: URL: https://github.com/apache/skywalking/pull/10296#issuecomment-1398122150 > > From my experience when users want to enable https they usually need a gateway to configure domain name and the port. Adding this to OAP makes it troublesome, for example, professional gateway can refresh expired cert without reboot but this is not possible in OAP for now as of your implementation. > > This is still my main concern, it doesn't look to be a production-ready implementation when you have to restart all the OAP instances when you want to renew the certificate You are correct. Certification update thing should be considered if you want to take security very seriously. But do we have a good way to do in Java and armeria? But I think this should not be a block. If users want to use a Gateway(we could document this for firehole-receiver), I am totally fine, and this feature would not block them to do so, right? In another way, our gRPC service seems to have the same issue(can't update cert in the runtime), right? We would not call that not product-ready. This reminds me, of when I wrote the security notice, https://skywalking.apache.org/docs/main/next/en/security/readme/. How many people would really put a Gateway before the SkyWalking OAP in the product? AFAIK, very few. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #10296: Enhance OAP HTTP server to support HTTPS
kezhenxu94 commented on PR #10296: URL: https://github.com/apache/skywalking/pull/10296#issuecomment-1398106425 > From my experience when users want to enable https they usually need a gateway to configure domain name and the port. Adding this to OAP makes it troublesome, for example, professional gateway can refresh expired cert without reboot but this is not possible in OAP for now as of your implementation. This is still my main concern, it doesn't look to be a production-ready implementation when you have to restart all the OAP instances when you want to renew the certificate -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
kezhenxu94 commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1082245628
##
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServer.java:
##
@@ -70,6 +74,22 @@ public void initialize() {
})
.decorator(LoggingService.newDecorator());
+if (config.isEnableTLS()) {
+sb.https(config.getPort());
Review Comment:
```suggestion
sb.https(new InetSocketAddress(
config.getHost(),
config.getPort()));
```
##
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServer.java:
##
@@ -70,6 +74,22 @@ public void initialize() {
})
.decorator(LoggingService.newDecorator());
+if (config.isEnableTLS()) {
+sb.https(config.getPort());
+try (InputStream cert =
+
Files.newInputStream(Paths.get(config.getTlsCertChainPath())
+ .toFile().toPath());
Review Comment:
```suggestion
try (InputStream cert = new
FileInputStream(config.getTlsCertChainPath());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking] mikechengwei commented on pull request #10298: feature: windows monitoring
mikechengwei commented on PR #10298: URL: https://github.com/apache/skywalking/pull/10298#issuecomment-1398019334 > This is an example of OTEL data mock for EKS monitoring. > > * https://github.com/apache/skywalking/tree/master/test/e2e-v2/cases/aws/eks > * Mock sender is here, https://github.com/apache/skywalking/blob/master/test/e2e-v2/java-test-service/e2e-mock-sender/src/main/java/org/apache/skywalking/e2e/controller/OtelMetricsSender.java ok, I thought I'd try to use the mock data way. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-php] wu-sheng merged pull request #48: Add authentication support.
wu-sheng merged PR #48: URL: https://github.com/apache/skywalking-php/pull/48 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] pg-yang commented on pull request #10296: Enhance OAP HTTP server to support HTTPS
pg-yang commented on PR #10296: URL: https://github.com/apache/skywalking/pull/10296#issuecomment-1398001600 > @pg-yang Have you tested this? I'm using gateway(nginx) to test firehouse https, rather than OAP. BTW, @yswdqz if you need domain,pulibc certificate, I could provide it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-php] jmjoy opened a new pull request, #48: Add authentication support.
jmjoy opened a new pull request, #48: URL: https://github.com/apache/skywalking-php/pull/48 1. Add configuration option `skywalking_agent.authentication`. 2. Update document. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-php] wu-sheng merged pull request #47: Add configuration option `skywalking_agent.runtime_dir`.
wu-sheng merged PR #47: URL: https://github.com/apache/skywalking-php/pull/47 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-php] jmjoy commented on a diff in pull request #47: Add configuration option `skywalking_agent.runtime_dir`.
jmjoy commented on code in PR #47: URL: https://github.com/apache/skywalking-php/pull/47#discussion_r1082157765 ## docs/en/setup/service-agent/php-agent/README.md: ## @@ -55,27 +55,31 @@ make install ## Configure php.ini ```ini -[skywalking] +[skywalking_agent] Review Comment: This tag isn't useful for php, but ours extension name is `skywalking_agent`, so `skywalking_agent` is more accurate. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-php] wu-sheng commented on a diff in pull request #47: Add configuration option `skywalking_agent.runtime_dir`.
wu-sheng commented on code in PR #47: URL: https://github.com/apache/skywalking-php/pull/47#discussion_r1082157018 ## docs/en/setup/service-agent/php-agent/README.md: ## @@ -55,27 +55,31 @@ make install ## Configure php.ini ```ini -[skywalking] +[skywalking_agent] Review Comment: So, we change from `skywalking` to `skywalking_agent` now? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-python] Superskyyy merged pull request #271: Add tables to show agent features and clean up configs
Superskyyy merged PR #271: URL: https://github.com/apache/skywalking-python/pull/271 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-php] jmjoy opened a new pull request, #47: Add configuration option `skywalking_agent.runtime_dir`.
jmjoy opened a new pull request, #47: URL: https://github.com/apache/skywalking-php/pull/47 1. Add configuration option `skywalking_agent.runtime_dir`, indicate the runtime directory for holding sock files now. 2. Update the document about option. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-python] wu-sheng commented on pull request #271: Add tables to show agent features and clean up configs
wu-sheng commented on PR #271: URL: https://github.com/apache/skywalking-python/pull/271#issuecomment-1397945914 It is just never defined from API level, and not implemented. Feel free to add the API and implement it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-python] Superskyyy commented on pull request #271: Add tables to show agent features and clean up configs
Superskyyy commented on PR #271: URL: https://github.com/apache/skywalking-python/pull/271#issuecomment-1397944719 @wu-sheng Btw, I remember HTTP handler (receiver) on meter is not implemented on the OAP side, is it intentional or just a missed thing not done? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-python] Superskyyy commented on pull request #271: Add tables to show agent features and clean up configs
Superskyyy commented on PR #271: URL: https://github.com/apache/skywalking-python/pull/271#issuecomment-1397941692 > I can't find meter and log docs, but the table shows supported. Are those missed docs? The log reporter doc is here, but meter is missing. It's because we didn't add it to the menu.yaml, adding now.  -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-python] wu-sheng commented on pull request #271: Add tables to show agent features and clean up configs
wu-sheng commented on PR #271: URL: https://github.com/apache/skywalking-python/pull/271#issuecomment-1397940344 I can't find meter and log docs, but the table shows supported. Are those missed docs? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
wu-sheng commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1082121473
##
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServerConfig.java:
##
@@ -39,4 +39,12 @@ public class HTTPServerConfig {
private int acceptQueueSize = 0;
@Builder.Default
private int maxRequestHeaderSize = 8192;
+
+@Builder.Default
+private boolean enableTLS = false;
+
+private int httpsPort;
Review Comment:
Yes, please.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking-python] Superskyyy opened a new pull request, #271: Add tables to show agent features and clean up configs
Superskyyy opened a new pull request, #271: URL: https://github.com/apache/skywalking-python/pull/271 As Python agent grows and syncing features with the Java agent, even myself don't remember what was supported especially we have 3 reporter protocols that not all of them support all telemetry data (grpc http kafka). So adding two tables to show it clearly for our users/developers so they can quick start and avoid installing the wrong version. Preview:  - [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue url. Closes: - [ ] Update the [`CHANGELOG.md`](https://github.com/apache/skywalking-python/blob/master/CHANGELOG.md). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] yswdqz commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
yswdqz commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1082120917
##
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServerConfig.java:
##
@@ -39,4 +39,12 @@ public class HTTPServerConfig {
private int acceptQueueSize = 0;
@Builder.Default
private int maxRequestHeaderSize = 8192;
+
+@Builder.Default
+private boolean enableTLS = false;
+
+private int httpsPort;
Review Comment:
For now I think use port is enough, because only `aws-receriver` need https,
and it only support https.
So I modify now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
wu-sheng commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1082107524
##
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServerConfig.java:
##
@@ -39,4 +39,12 @@ public class HTTPServerConfig {
private int acceptQueueSize = 0;
@Builder.Default
private int maxRequestHeaderSize = 8192;
+
+@Builder.Default
+private boolean enableTLS = false;
+
+private int httpsPort;
Review Comment:
I am wondering are we going to share tls and no tls server together? If not,
we could use port, rather than using a new port.
By this, I feel we open an insecure port for http.
It is better we don't expose that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10296: Enhance OAP HTTP server to support HTTPS
wu-sheng commented on PR #10296: URL: https://github.com/apache/skywalking/pull/10296#issuecomment-1397912275 @pg-yang Have you tested this? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] yswdqz commented on pull request #10296: Enhance OAP HTTP server to support HTTPS
yswdqz commented on PR #10296: URL: https://github.com/apache/skywalking/pull/10296#issuecomment-1397909868 Is there anything else I should change? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10298: feature: windows monitoring
wu-sheng commented on PR #10298: URL: https://github.com/apache/skywalking/pull/10298#issuecomment-1397894118 This is an example of OTEL data mock for EKS monitoring. - https://github.com/apache/skywalking/tree/master/test/e2e-v2/cases/aws/eks - Mock sender is here, https://github.com/apache/skywalking/blob/master/test/e2e-v2/java-test-service/e2e-mock-sender/src/main/java/org/apache/skywalking/e2e/controller/OtelMetricsSender.java -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #10298: feature: windows monitoring
kezhenxu94 commented on PR #10298: URL: https://github.com/apache/skywalking/pull/10298#issuecomment-1397891241 Anyway, if you have a Windows machine, just try to run the e2e and if it fail then switch to the mocking method -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] mikechengwei commented on pull request #10298: feature: windows monitoring
mikechengwei commented on PR #10298: URL: https://github.com/apache/skywalking/pull/10298#issuecomment-1397888119  windows_exporter also use windows operate system to run e2e test. @kezhenxu94 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10298: feature: windows monitoring
wu-sheng commented on PR #10298: URL: https://github.com/apache/skywalking/pull/10298#issuecomment-1397882042 > In short, you can run the Windows system in a Docker container and monitor that system inside the container, no need to run the e2e on a real Windows system I am afraid Windows is not in that case. You could run a Linux container on Windows, but can't be reversed. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #10298: feature: windows monitoring
kezhenxu94 commented on PR #10298: URL: https://github.com/apache/skywalking/pull/10298#issuecomment-1397881095 > You could use a window server for CI. > > https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources > > @kezhenxu94 Does our e2e support Windows? I don't see any need to run the e2e on Windows, just refer to the existing e2e for Linux monitoring https://github.com/apache/skywalking/blob/263c2b442c8b9318d839d868b503bc61c8ed6eb7/test/e2e-v2/cases/vm/prometheus-node-exporter/Dockerfile.nodeExporter#L16-L21 and replace the base Docker image with a [Windows one](https://mcr.microsoft.com/en-us/product/windows/tags), replace the exporter to a [Windows exporter](https://github.com/prometheus-community/windows_exporter/releases/download/v0.20.0/windows_exporter-0.20.0-386.exe) In short, you can run the Windows system in a Docker container and monitor that system inside the container, no need to run the e2e on a real Windows system -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10298: feature: windows monitoring
wu-sheng commented on PR #10298: URL: https://github.com/apache/skywalking/pull/10298#issuecomment-1397877993 I am feeling there are tools we can't support on Windows. @pg-yang Could you share your OTEL mock service? I think we need to use that to mock the telemetry on Windows. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10298: feature: windows monitoring
wu-sheng commented on PR #10298: URL: https://github.com/apache/skywalking/pull/10298#issuecomment-1397877093 You could use a window server for CI. https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources @kezhenxu94 Does our e2e support Windows? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] mikechengwei commented on pull request #10298: feature: windows monitoring
mikechengwei commented on PR #10298: URL: https://github.com/apache/skywalking/pull/10298#issuecomment-1397875061 @wankai123 I find windows image can't build and run in linux physical machine，it will throw exception below ``` docker: no matching manifest for linux/amd64 in the manifest list entries. ``` Ref: https://stackoverflow.com/questions/42158596/can-windows-containers-be-hosted-on-linux I think our CI machine is linux, right? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on a diff in pull request #241: fix metadata register bug.
lujiajing1126 commented on code in PR #241:
URL:
https://github.com/apache/skywalking-banyandb/pull/241#discussion_r1082069269
##
api/proto/banyandb/database/v1/schema.proto:
##
@@ -88,25 +89,25 @@ enum CompressionMethod {
// FieldSpec is the specification of field
message FieldSpec {
// name is the identity of a field
- string name = 1;
+ string name = 1 [(validate.rules).string.min_len = 1];
// field_type denotes the type of field value
- FieldType field_type = 2;
+ FieldType field_type = 2 [(validate.rules).enum.defined_only = true];
// encoding_method indicates how to encode data during writing
- EncodingMethod encoding_method = 3;
+ EncodingMethod encoding_method = 3 [(validate.rules).enum.defined_only =
true];
// compression_method indicates how to compress data during writing
- CompressionMethod compression_method = 4;
+ CompressionMethod compression_method = 4 [(validate.rules).enum.defined_only
= true];
}
// Measure intends to store data point
message Measure {
// metadata is the identity of a measure
- common.v1.Metadata metadata = 1;
+ common.v1.Metadata metadata = 1 [(validate.rules).message.required = true];
// tag_families are for filter measures
- repeated TagFamilySpec tag_families = 2;
+ repeated TagFamilySpec tag_families = 2 [(validate.rules).repeated.min_items
= 1];
// fields denote measure values
repeated FieldSpec fields = 3;
// entity indicates which tags will be to generate a series and shard a
measure
- Entity entity = 4;
+ Entity entity = 4 [(validate.rules).message.required = true];
// interval indicates how frequently to send a data point
// valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h", "d".
string interval = 5;
Review Comment:
Is that possbile to validate interval with regex?
##
banyand/metadata/schema/group.go:
##
@@ -87,6 +88,9 @@ func (e *etcdSchemaRegistry) DeleteGroup(ctx context.Context,
group string) (boo
}
func (e *etcdSchemaRegistry) CreateGroup(ctx context.Context, group
*commonv1.Group) error {
+ if group.UpdatedAt != nil {
Review Comment:
I suppose we can move `UpdatedAt` field assignment to the `create` and
`update` methods
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking] mikechengwei commented on pull request #10298: feature: windows monitoring
mikechengwei commented on PR #10298: URL: https://github.com/apache/skywalking/pull/10298#issuecomment-1397866600 > > The E2E is running on docker, you can use the wins_exporter image to run it. > > Or you can base on `windows/nanoserver` to build a image, if you can't find a official one. This might help: https://github.com/prometheus-community/windows_exporter/blob/master/Dockerfile Ok. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] wankai123 commented on pull request #10298: feature: windows monitoring
wankai123 commented on PR #10298: URL: https://github.com/apache/skywalking/pull/10298#issuecomment-1397865868 > The E2E is running on docker, you can use the wins_exporter image to run it. Or you can base on `windows/nanoserver` to build a image, if you can't find a official one. This might help: https://github.com/prometheus-community/windows_exporter/blob/master/Dockerfile -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] mikechengwei commented on pull request #10298: feature: windows monitoring
mikechengwei commented on PR #10298: URL: https://github.com/apache/skywalking/pull/10298#issuecomment-1397857748 > wins_exporter ok, i get. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] wankai123 commented on pull request #10298: feature: windows monitoring
wankai123 commented on PR #10298: URL: https://github.com/apache/skywalking/pull/10298#issuecomment-1397856615 > > The expressions LGTM, let's wait the e2e codes and others to finish this. > > I have a question to ask you, i need install wins_exporter in a windows machine implement in e2e test , how should I implement this logic? @wankai123 The E2E is running on docker, you can use the wins_exporter image to run it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] mikechengwei commented on pull request #10298: feature: windows monitoring
mikechengwei commented on PR #10298: URL: https://github.com/apache/skywalking/pull/10298#issuecomment-1397855253 > The expressions LGTM, let's wait the e2e codes and others to finish this. I have a question to ask you, i need install wins_exporter in a windows machine implement in e2e test , how should I implement this logic? @wankai123 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] mikechengwei commented on pull request #10298: feature: windows monitoring
mikechengwei commented on PR #10298: URL: https://github.com/apache/skywalking/pull/10298#issuecomment-1397824068 > Take your time. I just pointed out what may be missed as you have labeled this PR ready for review. :) Thanks for your trust, I will improve this pr as soon as possible. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-php] Superskyyy commented on pull request #46: Report instance properties and keep alive.
Superskyyy commented on PR #46: URL: https://github.com/apache/skywalking-php/pull/46#issuecomment-1397797583 > Yes, you are correct. I totally forgot this. 😅 😄 @jmjoy FYI the instance property will require a periodical update. Refer to Java agent here: https://github.dev/apache/skywalking-java/blob/f366503d58aa7e3395d88f6c70e6ea885fc50d2e/apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/remote/ServiceManagementClient.java#L105 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-php] wu-sheng commented on pull request #46: Report instance properties and keep alive.
wu-sheng commented on PR #46: URL: https://github.com/apache/skywalking-php/pull/46#issuecomment-1397755751 Yes, you are correct. I totally forgot this. 😅 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-php] Superskyyy commented on pull request #46: Report instance properties and keep alive.
Superskyyy commented on PR #46: URL: https://github.com/apache/skywalking-php/pull/46#issuecomment-1397591193 I just saw this and remembered agents were supposed to resubmit instance properties periodically as the OAP TTL mechanism would purge them. Is it still true? @wu-sheng -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10298: feature: windows monitoring
wu-sheng commented on PR #10298: URL: https://github.com/apache/skywalking/pull/10298#issuecomment-1397113669 FYI @Donadominic -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10298: feature: windows monitoring
wu-sheng commented on PR #10298: URL: https://github.com/apache/skywalking/pull/10298#issuecomment-1397112050 > OK, I just successfully ran through the process, I will continue to improve the pr. @wu-sheng Take your time. I just pointed out which may be missed as you have labeled this PR ready for review. :) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] wankai123 commented on a diff in pull request #10298: feature: windows monitoring
wankai123 commented on code in PR #10298:
URL: https://github.com/apache/skywalking/pull/10298#discussion_r1081373409
##
oap-server/server-starter/src/main/resources/otel-rules/windows.yaml:
##
@@ -0,0 +1,72 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You 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.
+
+# This will parse a textual representation of a duration. The formats
+# accepted are based on the ISO-8601 duration format {@code PnDTnHnMn.nS}
+# with days considered to be exactly 24 hours.
+#
+# Examples:
+#
+#"PT20.345S" -- parses as "20.345 seconds"
+#"PT15M" -- parses as "15 minutes" (where a minute is 60 seconds)
+#"PT10H" -- parses as "10 hours" (where an hour is 3600 seconds)
+#"P2D" -- parses as "2 days" (where a day is 24 hours or 86400
seconds)
+#"P2DT3H4M" -- parses as "2 days, 3 hours and 4 minutes"
+#"P-6H3M"-- parses as "-6 hours and +3 minutes"
+#"-P6H3M"-- parses as "-6 hours and -3 minutes"
+#"-P-6H+3M" -- parses as "+6 hours and -3 minutes"
+#
+filter: "{ tags -> tags.job_name == 'windows-monitoring' }" # The
OpenTelemetry job name
+expSuffix: service(['node_identifier_host_name'] , Layer.OS_WINDOWS)
+metricPrefix: meter_win
+metricsRules:
+ #cpu windows don't expose cpu load metrics
+ - name: cpu_total_percentage
+exp: (windows_cpu_time_total * 100).tagNotEqual('mode' ,
'idle').sum(['node_identifier_host_name']).rate('PT1M')
+ - name: cpu_average_used
+exp: (windows_cpu_time_total * 100).sum(['node_identifier_host_name' ,
'mode']).rate('PT1M')
+
+ #memory
+ - name: memory_total
+exp: windows_cs_physical_memory_bytes
+ - name: memory_available
+exp: windows_os_physical_memory_free_bytes
+ - name: memory_used
+exp: windows_cs_physical_memory_bytes-windows_os_physical_memory_free_bytes
Review Comment:
```suggestion
exp: windows_cs_physical_memory_bytes -
windows_os_physical_memory_free_bytes
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking] mikechengwei commented on pull request #10298: feature: windows monitoring
mikechengwei commented on PR #10298: URL: https://github.com/apache/skywalking/pull/10298#issuecomment-1397075052 https://user-images.githubusercontent.com/13257127/213469648-dd5fbf7a-e3c8-4eb4-9e8a-a3dd688f23b3.png";> OK, I just successfully ran through the process, I will continue to improve the pr. @wu-sheng -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
wu-sheng commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1081347271
##
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServerConfig.java:
##
@@ -39,4 +39,14 @@ public class HTTPServerConfig {
private int acceptQueueSize = 0;
@Builder.Default
private int maxRequestHeaderSize = 8192;
+
+@Builder.Default
+private boolean enableTLS = false;
+@Builder.Default
+private boolean enableTlsSelfSigned = false;
+
+private int httpsPort;
+private String tlsKeyPath;
+private String tlsCertChainPath;
Review Comment:
We don't need to add a domain name for now. But `enableTlsSelfSigned` seems
to need to be removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking] yswdqz commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
yswdqz commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1081345389
##
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServerConfig.java:
##
@@ -39,4 +39,14 @@ public class HTTPServerConfig {
private int acceptQueueSize = 0;
@Builder.Default
private int maxRequestHeaderSize = 8192;
+
+@Builder.Default
+private boolean enableTLS = false;
+@Builder.Default
+private boolean enableTlsSelfSigned = false;
+
+private int httpsPort;
+private String tlsKeyPath;
+private String tlsCertChainPath;
Review Comment:
So I don't need to change configuration item now ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10298: feature: windows monitoring
wu-sheng commented on PR #10298: URL: https://github.com/apache/skywalking/pull/10298#issuecomment-1397067644 We should build a CI on GHA/Windows to test and verify the metrics. And we could provide the OTEL configuration you provided. Further, we should have a dashboard setup for Windows, including UI side updates to add a menu item. The last thing, the official dashboard docs should be added. You could refer to the Linux one. - https://skywalking.apache.org/docs/main/next/en/setup/backend/backend-vm-monitoring/ - https://skywalking.apache.org/docs/main/next/en/setup/backend/opentelemetry-receiver/ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] mikechengwei opened a new pull request, #10298: feature: windows monitoring
mikechengwei opened a new pull request, #10298: URL: https://github.com/apache/skywalking/pull/10298 - [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #10242 . - [ ] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/docs/en/changes/changes.md). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
wu-sheng commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1081338049
##
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServerConfig.java:
##
@@ -39,4 +39,14 @@ public class HTTPServerConfig {
private int acceptQueueSize = 0;
@Builder.Default
private int maxRequestHeaderSize = 8192;
+
+@Builder.Default
+private boolean enableTLS = false;
+@Builder.Default
+private boolean enableTlsSelfSigned = false;
+
+private int httpsPort;
+private String tlsKeyPath;
+private String tlsCertChainPath;
Review Comment:
> But still, we need to make it clear, do we support https for all other
http server like Prometheus, Zipkin receiver, or do we want to only support
that for AWS receiver? If we want to support for all, we need these
configurations for each server respectively, otherwise we should move these
configurations to that plugin.
For now, we plan to build a new HTTPS receiver inside
`aws-firehose-receiver` separate from the existing core and sharing server HTTP
server, also separate from Prometheus or Zipkin server.
We don't need to support all in HTTPS, at least not for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking-banyandb] hanahmily commented on a diff in pull request #239: Unify TagFilter for Streaming scenario
hanahmily commented on code in PR #239:
URL:
https://github.com/apache/skywalking-banyandb/pull/239#discussion_r1081318960
##
banyand/liaison/grpc/registry.go:
##
@@ -435,7 +437,37 @@ type topNAggregationRegistryServer struct {
func (ts *topNAggregationRegistryServer) Create(ctx context.Context,
req *databasev1.TopNAggregationRegistryServiceCreateRequest,
) (*databasev1.TopNAggregationRegistryServiceCreateResponse, error) {
- if err :=
ts.schemaRegistry.TopNAggregationRegistry().CreateTopNAggregation(ctx,
req.GetTopNAggregation()); err != nil {
+ topNSchema := req.GetTopNAggregation()
Review Comment:
Move the measure creation operation into `topNProcessorManager.start`.
Liaison is a proxy and should not implement complicated functions.
If there is no such measure, the manager should create a new one. The
manager should check the on-disk measure's specs. If it's different from
expectation, it could get updated.
##
banyand/liaison/grpc/registry.go:
##
@@ -435,7 +437,37 @@ type topNAggregationRegistryServer struct {
func (ts *topNAggregationRegistryServer) Create(ctx context.Context,
req *databasev1.TopNAggregationRegistryServiceCreateRequest,
) (*databasev1.TopNAggregationRegistryServiceCreateResponse, error) {
- if err :=
ts.schemaRegistry.TopNAggregationRegistry().CreateTopNAggregation(ctx,
req.GetTopNAggregation()); err != nil {
+ topNSchema := req.GetTopNAggregation()
+ if err :=
ts.schemaRegistry.TopNAggregationRegistry().CreateTopNAggregation(ctx,
topNSchema); err != nil {
+ return nil, err
+ }
+
+ sourceMeasure, err :=
ts.schemaRegistry.MeasureRegistry().GetMeasure(ctx,
topNSchema.GetSourceMeasure())
+ if err != nil {
+ return nil, err
+ }
+ // create a "virtual" measure for TopN result
+ topNMeasure := &databasev1.Measure{
+ Metadata: topNSchema.GetMetadata(),
+ Interval: sourceMeasure.GetInterval(),
+ TagFamilies: []*databasev1.TagFamilySpec{
+ {
+ Name: measure.TopNTagFamily,
+ Tags: []*databasev1.TagSpec{
+ {
+ Name: "measureID",
Review Comment:
use a snake case here: `measure_id`
##
banyand/liaison/grpc/registry.go:
##
@@ -453,12 +485,13 @@ func (ts *topNAggregationRegistryServer) Update(ctx
context.Context,
func (ts *topNAggregationRegistryServer) Delete(ctx context.Context,
req *databasev1.TopNAggregationRegistryServiceDeleteRequest,
) (*databasev1.TopNAggregationRegistryServiceDeleteResponse, error) {
- ok, err :=
ts.schemaRegistry.TopNAggregationRegistry().DeleteTopNAggregation(ctx,
req.GetMetadata())
- if err != nil {
+ okTopNDeleted, errDelTopN :=
ts.schemaRegistry.TopNAggregationRegistry().DeleteTopNAggregation(ctx,
req.GetMetadata())
+ okMeasureDeleted, errDelMeasure :=
ts.schemaRegistry.MeasureRegistry().DeleteMeasure(ctx, req.GetMetadata())
Review Comment:
We don't have to support the cascade deletion. We don't introduce
transactional operations. The operation is too dangerous.
##
banyand/liaison/grpc/registry.go:
##
@@ -435,7 +437,37 @@ type topNAggregationRegistryServer struct {
func (ts *topNAggregationRegistryServer) Create(ctx context.Context,
req *databasev1.TopNAggregationRegistryServiceCreateRequest,
) (*databasev1.TopNAggregationRegistryServiceCreateResponse, error) {
- if err :=
ts.schemaRegistry.TopNAggregationRegistry().CreateTopNAggregation(ctx,
req.GetTopNAggregation()); err != nil {
+ topNSchema := req.GetTopNAggregation()
+ if err :=
ts.schemaRegistry.TopNAggregationRegistry().CreateTopNAggregation(ctx,
topNSchema); err != nil {
+ return nil, err
+ }
+
+ sourceMeasure, err :=
ts.schemaRegistry.MeasureRegistry().GetMeasure(ctx,
topNSchema.GetSourceMeasure())
+ if err != nil {
+ return nil, err
+ }
+ // create a "virtual" measure for TopN result
Review Comment:
"virtual" is a common notion. How about using `derived` or `corresponding`
instead?
##
banyand/measure/measure_write.go:
##
@@ -39,17 +39,18 @@ import (
var errMalformedElement = errors.New("element is malformed")
-func (s *measure) write(md *commonv1.Metadata, shardID common.ShardID, entity
[]byte, entityValues tsdb.EntityValues, value *measurev1.DataPointValue) error {
+func (s *measure) write(sm *databasev1.Measure, shardID common.ShardID, entity
[]byte, entityValues tsdb.EntityValues,
Review Comment:
You had better remove the first parameter `sm`. We could get it from `s`
directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
kezhenxu94 commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1081327623
##
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServerConfig.java:
##
@@ -39,4 +39,14 @@ public class HTTPServerConfig {
private int acceptQueueSize = 0;
@Builder.Default
private int maxRequestHeaderSize = 8192;
+
+@Builder.Default
+private boolean enableTLS = false;
+@Builder.Default
+private boolean enableTlsSelfSigned = false;
+
+private int httpsPort;
+private String tlsKeyPath;
+private String tlsCertChainPath;
Review Comment:
But still, we need to make it clear, do we support https for all other http
server like Prometheus, Zipkin receiver, or do we want to only support that for
AWS receiver? If we want to support for all, we need these configurations for
each server respectively, otherwise we should move these configurations to that
plugin.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
kezhenxu94 commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1081322512
##
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServerConfig.java:
##
@@ -39,4 +39,14 @@ public class HTTPServerConfig {
private int acceptQueueSize = 0;
@Builder.Default
private int maxRequestHeaderSize = 8192;
+
+@Builder.Default
+private boolean enableTLS = false;
+@Builder.Default
+private boolean enableTlsSelfSigned = false;
+
+private int httpsPort;
+private String tlsKeyPath;
+private String tlsCertChainPath;
Review Comment:
We are actually using the server host here
https://github.com/apache/skywalking/blob/289427990aa857298e498782ad97a1d8db68ae4a/oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/grpc/GRPCServer.java#L86
After thinking twice I think I was wrong, we don't need to configure the
host/domain name, as long as the CN field in the cert is the same as the
hostname that are used to access this endpoint, we are good to go.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #9883: [Feature] Support AWS service monitoring
wu-sheng commented on issue #9883: URL: https://github.com/apache/skywalking/issues/9883#issuecomment-1397040340 That is good. We should be able to have public static IP on EC2. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on issue #9883: [Feature] Support AWS service monitoring
kezhenxu94 commented on issue #9883:
URL: https://github.com/apache/skywalking/issues/9883#issuecomment-1397037616
> By my test, the self-signed certificate will encounter errors such as:
>
> ```
>
> {
>
> "deliveryStreamARN": "",
>
> "destination": "35",
>
> "deliveryStreamVersionId": 1,
>
> "message": "Unable to complete an SSL Handshake with the endpoint due
to invalid certification path. Contact the owner of the endpoint to resolve
this issue.",
>
> "errorCode": "HttpEndpoint.SSLHandshakeCertificatePathFailure"
>
> }
>
> ```
>
> I will register a domain and public certificate to test firehouse. It also
is required for testing AWS monitoring.
FWIW, [ZeroSSL](https://zerossl.com/) can issue certificates for an IP
address, if you can run the OAP on a machine with a public IP, you can also
test it with firehouse.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking] pg-yang commented on issue #9883: [Feature] Support AWS service monitoring
pg-yang commented on issue #9883:
URL: https://github.com/apache/skywalking/issues/9883#issuecomment-1397013267
By my test, the self-signed certificate will encounter errors such as:
```
{
"deliveryStreamARN": "",
"destination": "35",
"deliveryStreamVersionId": 1,
"message": "Unable to complete an SSL Handshake with the endpoint due to
invalid certification path. Contact the owner of the endpoint to resolve this
issue.",
"errorCode": "HttpEndpoint.SSLHandshakeCertificatePathFailure"
}
```
I will register a domain and public certificate to test firehouse. It also
is required for testing AWS monitoring.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
wu-sheng commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1081223732
##
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServerConfig.java:
##
@@ -39,4 +39,14 @@ public class HTTPServerConfig {
private int acceptQueueSize = 0;
@Builder.Default
private int maxRequestHeaderSize = 8192;
+
+@Builder.Default
+private boolean enableTLS = false;
+@Builder.Default
+private boolean enableTlsSelfSigned = false;
+
+private int httpsPort;
+private String tlsKeyPath;
+private String tlsCertChainPath;
Review Comment:
https://user-images.githubusercontent.com/5441976/213448204-3d859dc6-690c-4532-b51c-27f3732c9bc6.png";>
I mean we have relative/similar configurations in gRPC part.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
kezhenxu94 commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1081123339
##
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServerConfig.java:
##
@@ -39,4 +39,14 @@ public class HTTPServerConfig {
private int acceptQueueSize = 0;
@Builder.Default
private int maxRequestHeaderSize = 8192;
+
+@Builder.Default
+private boolean enableTLS = false;
+@Builder.Default
+private boolean enableTlsSelfSigned = false;
+
+private int httpsPort;
+private String tlsKeyPath;
+private String tlsCertChainPath;
Review Comment:
All these configurations are domain-name:port specific, i.e. every domain
name:port should have the corresponding tlsKeyPath, and tlsCertChainPath.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
kezhenxu94 commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1081217976
##
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServerConfig.java:
##
@@ -39,4 +39,14 @@ public class HTTPServerConfig {
private int acceptQueueSize = 0;
@Builder.Default
private int maxRequestHeaderSize = 8192;
+
+@Builder.Default
+private boolean enableTLS = false;
+@Builder.Default
+private boolean enableTlsSelfSigned = false;
+
+private int httpsPort;
+private String tlsKeyPath;
+private String tlsCertChainPath;
Review Comment:
> One question, today's gRPC seems not setting the domain name?
We have no built-in https support in today's gRPC server inside OAP. In
another word, users have to setup a gateway to support https for gRPC port
11800.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
wu-sheng commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1081209069
##
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServerConfig.java:
##
@@ -39,4 +39,14 @@ public class HTTPServerConfig {
private int acceptQueueSize = 0;
@Builder.Default
private int maxRequestHeaderSize = 8192;
+
+@Builder.Default
+private boolean enableTLS = false;
+@Builder.Default
+private boolean enableTlsSelfSigned = false;
+
+private int httpsPort;
+private String tlsKeyPath;
+private String tlsCertChainPath;
Review Comment:
One question, today's gRPC seems not setting the domain name?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
kezhenxu94 commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1081116842
##
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServer.java:
##
@@ -70,6 +78,21 @@ public void initialize() {
})
.decorator(LoggingService.newDecorator());
+if (config.isEnableTLS()) {
+sb.https(config.getHttpsPort());
+if (config.isEnableTlsSelfSigned()) {
+sb.tlsSelfSigned();
Review Comment:
It's impossible to use self signed certificate for services that are exposed
to external consumer, i.e. aws won't definitely skip verify certificate like
you do in the unit tests, so I think this is useless in reality.
##
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServerConfig.java:
##
@@ -39,4 +39,14 @@ public class HTTPServerConfig {
private int acceptQueueSize = 0;
@Builder.Default
private int maxRequestHeaderSize = 8192;
+
+@Builder.Default
+private boolean enableTLS = false;
+@Builder.Default
+private boolean enableTlsSelfSigned = false;
+
+private int httpsPort;
+private String tlsKeyPath;
+private String tlsCertChainPath;
Review Comment:
All these configurations are domain-name:port specific, i.e. every domain
name:port should have the corresponding https port, tlsKeyPath,
tlsCertChainPath.
##
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServer.java:
##
@@ -70,6 +78,21 @@ public void initialize() {
})
.decorator(LoggingService.newDecorator());
+if (config.isEnableTLS()) {
+sb.https(config.getHttpsPort());
+if (config.isEnableTlsSelfSigned()) {
+sb.tlsSelfSigned();
+} else {
+try (InputStream cert =
+
Files.newInputStream(Paths.get(config.getTlsCertChainPath())
+ .toFile().toPath());
+ InputStream key =
PrivateKeyUtil.loadDecryptionKey(config.getTlsKeyPath())) {
+sb.tls(cert, key);
Review Comment:
Https certificates are bound to a domain name so you have to add a
configuration item to configure virtual host for that cert.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10296: Enhance OAP HTTP server to support HTTPS
wu-sheng commented on PR #10296: URL: https://github.com/apache/skywalking/pull/10296#issuecomment-1396810825 > I have not found docs about httpserver. Is there a doc I need to edit? No, there isn't doc for a class. But there are core and sharing server modules using this, if you are going to expose configurations(currently you seem not), you need to update relative docs. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] yswdqz commented on pull request #10296: Enhance OAP HTTP server to support HTTPS
yswdqz commented on PR #10296: URL: https://github.com/apache/skywalking/pull/10296#issuecomment-1396765973 I have not found docs about httpserver. Is there a doc I need to edit? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] yswdqz opened a new pull request, #10296: Add https
yswdqz opened a new pull request, #10296: URL: https://github.com/apache/skywalking/pull/10296 - [ ] If this is non-trivial feature, paste the links/URLs to the design doc. - [] Update the documentation to include this new feature. - [x] Tests(including UT, IT, E2E) are added to verify the new feature. - [ ] If it's UI related, attach the screenshots below. - [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #. - [x] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/docs/en/changes/changes.md). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on pull request #239: Unify TagFilter for Streaming scenario
lujiajing1126 commented on PR #239: URL: https://github.com/apache/skywalking-banyandb/pull/239#issuecomment-1396703190 I've refactored the wring path of `TopN Aggregation` results by creating new internal writing request to the pipeline. And every TopN schema is now bounded to a real Measure schema which is used to listen and write TopN entities. The virtual/real measure concepts are still there since the measure of `TopN` is incomplete. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-php] wu-sheng merged pull request #46: Report instance properties and keep alive.
wu-sheng merged PR #46: URL: https://github.com/apache/skywalking-php/pull/46 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on a diff in pull request #239: Unify TagFilter for Streaming scenario
lujiajing1126 commented on code in PR #239:
URL:
https://github.com/apache/skywalking-banyandb/pull/239#discussion_r1081016963
##
banyand/measure/measure_write.go:
##
@@ -60,7 +61,7 @@ func (s *measure) write(md *commonv1.Metadata, shardID
common.ShardID, entity []
if err != nil {
return err
}
- ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ ctx, cancel := context.WithTimeout(context.TODO(), 5*time.Second)
Review Comment:
There is no difference between `TODO` and `Background` from the
implementation. Anyway, reverted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking-php] jmjoy opened a new pull request, #46: Report instance properties and keep alive.
jmjoy opened a new pull request, #46: URL: https://github.com/apache/skywalking-php/pull/46 1. Report instance properties and keep alive. 2. Clearly limit the version of Skywalking (>= 8). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #9883: [Feature] Support AWS service monitoring
wu-sheng commented on issue #9883: URL: https://github.com/apache/skywalking/issues/9883#issuecomment-1396562092 Does this http server listener for awsreceiver only?(I assume so)? Then it should be a new one. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] yswdqz commented on issue #9883: [Feature] Support AWS service monitoring
yswdqz commented on issue #9883: URL: https://github.com/apache/skywalking/issues/9883#issuecomment-1396556253 @wu-sheng Which port should we open as https port? Should it be defined by the user, or should it be located on the same port as http? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-rover] mrproliu commented on pull request #73: Try to support OpenSSL 3.0.x
mrproliu commented on PR #73: URL: https://github.com/apache/skywalking-rover/pull/73#issuecomment-1396530028 Thanks for your contribution to support OpenSSL `3.0.x`. Could you also update the change log? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-rover] mrproliu merged pull request #74: Add the syscall level event to the trace
mrproliu merged PR #74: URL: https://github.com/apache/skywalking-rover/pull/74 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-rover] mrproliu commented on pull request #74: Add the syscall level event to the trace
mrproliu commented on PR #74: URL: https://github.com/apache/skywalking-rover/pull/74#issuecomment-1396471270 > lgtm. I am not sure whether we have docs to update here and/or main repo? We should list which events we could collect and attach to spans. Updated in the documentation. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #10295: [Feature] [UI] Improve the clarity of event list display in trace span
wu-sheng commented on issue #10295: URL: https://github.com/apache/skywalking/issues/10295#issuecomment-1396468297 We just need to reuse similar UI component, it doesn't need to a span. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] mrproliu opened a new issue, #10295: [Feature] [UI] Improve the clarity of event list display in trace span
mrproliu opened a new issue, #10295: URL: https://github.com/apache/skywalking/issues/10295 ### Search before asking - [X] I had searched in the [issues](https://github.com/apache/skywalking/issues?q=is%3Aissue) and found no similar feature requirement. ### Description Change the event list in trace span to Trace view ### Use case Currently, the event list displayed in trace spans is not clear and easy to understand. We suggest implementing a trace display mode where the current span is used as the root span, and event names are displayed as span names. Additionally, the events should be sorted based on their start time (in nanoseconds) to form a trace tree. This way, the names of the events will be easily visible on the left side, and the execution time will be easily visible on the right side. This will greatly improve the overall visibility and clarity of the trace spans. ### Related issues _No response_ ### Are you willing to submit a PR? - [ ] Yes I am willing to submit a PR! ### Code of Conduct - [X] I agree to follow this project's [Code of Conduct](https://www.apache.org/foundation/policies/conduct) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking] mufiye commented on issue #9999: [Feature] [Java] Expose complete Tracing APIs in the tracing toolkit
mufiye commented on issue #: URL: https://github.com/apache/skywalking/issues/#issuecomment-1396418646 > I mean, as you have enhanced one class successfully, there would not be special cases that could be enhanced or not. :) I think that was driving you to ask. My point is, don't treat this as a very complex thing, it is just simple enhancing and intercepting. Ok, I get it now. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-rover] mrproliu opened a new pull request, #74: Add the syscall level event to the trace
mrproliu opened a new pull request, #74: URL: https://github.com/apache/skywalking-rover/pull/74 Including the following summary of the event: 1. Network interface information 2. RTT metrics 3. Package Count/Size 4. Syscall function name(write, read, etc.) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
