Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for logEntry->traceSampled field extraction. #297

Merged
merged 4 commits into from
Feb 28, 2019
Merged

Add support for logEntry->traceSampled field extraction. #297

merged 4 commits into from
Feb 28, 2019

Conversation

qingling128
Copy link
Contributor

No description provided.

@qingling128
Copy link
Contributor Author

Fixed Travis

igorpeshansky
igorpeshansky requested changes Feb 27, 2019
View reviewed changes
lib/fluent/plugin/out_google_cloud.rb
@@ -643,7 +647,8 @@ def write(chunk)
# Propagate these if necessary. Note that we don't want to
# override these keys in the JSON we've just parsed.
preserved_keys.each do |key|
record_json[key] ||= record[key] if record.key?(key)
record_json[key] ||= record[key] if
record.key?(key) && !record_json.key?(key)
Copy link
Member

Choose a reason for hiding this comment

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

||= should take care of this, unless you are trying to preserve nil values...

Copy link
Contributor Author

@qingling128 qingling128 Feb 27, 2019 •
edited by igorpeshansky

Choose a reason for hiding this comment

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

The problem with ||= is that it changed a boolean from false to true... which is what we want to avoid. We only want to override it if it's not present in the JSON.

$ irb
2.4.3 :001 > var = false
 => false
2.4.3 :002 > var ||= true
 => true

test/plugin/base_test.rb Outdated
assert_equal expected_value, entry[log_entry_field],
"Index #{index} failed. #{expected_value} is expected" \
" for #{log_entry_field} field."
if default_value.nil?
Copy link
Member

Choose a reason for hiding this comment

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

I think you should be able to just use assert_equal_with_default here, even when the default is nil, since proto values will never produce nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

qingling128
qingling128 commented Feb 28, 2019
View reviewed changes
Copy link
Contributor Author

@qingling128 qingling128 left a comment

Choose a reason for hiding this comment

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

PTAL

lib/fluent/plugin/out_google_cloud.rb
@@ -643,7 +647,8 @@ def write(chunk)
# Propagate these if necessary. Note that we don't want to
# override these keys in the JSON we've just parsed.
preserved_keys.each do |key|
record_json[key] ||= record[key] if record.key?(key)
record_json[key] ||= record[key] if
record.key?(key) && !record_json.key?(key)
Copy link
Contributor Author

@qingling128 qingling128 Feb 27, 2019 •
edited by igorpeshansky

Choose a reason for hiding this comment

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

The problem with ||= is that it changed a boolean from false to true... which is what we want to avoid. We only want to override it if it's not present in the JSON.

$ irb
2.4.3 :001 > var = false
 => false
2.4.3 :002 > var ||= true
 => true

test/plugin/base_test.rb Outdated
assert_equal expected_value, entry[log_entry_field],
"Index #{index} failed. #{expected_value} is expected" \
" for #{log_entry_field} field."
if default_value.nil?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

igorpeshansky
igorpeshansky approved these changes Feb 28, 2019
View reviewed changes
Copy link
Member

@igorpeshansky igorpeshansky left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@qingling128 qingling128 merged commit e07aecc into master Feb 28, 2019
@qingling128 qingling128 deleted the lingshi-trace-sampled branch February 28, 2019 00:16
sebright added a commit to sebright/opencensus-experiments that referenced this pull request Mar 1, 2019
@sebright
Use Stackdriver special field to set sampling decision in log correla…
9126658 
…tion demo.

This commit sets the new sampling decision field that is recognized by the
Stackdriver Logging agent, "logging.googleapis.com/traceSampled", in the Log4j
log correlation demo.  The sampling decision field was added in
GoogleCloudPlatform/fluent-plugin-google-cloud#297.
@sebright sebright mentioned this pull request Mar 1, 2019
Merged
sebright added a commit to sebright/microservices-demo that referenced this pull request Mar 1, 2019
@sebright
adservice: use Stackdriver special field to set sampling decision in …
edc1186 
…log entries

This commit sets the new sampling decision field that is recognized by the
Stackdriver Logging agent, "logging.googleapis.com/traceSampled".  The sampling
decision field was added in
GoogleCloudPlatform/fluent-plugin-google-cloud#297, and
it won't be available until the new version of fluent-plugin-google-cloud is
used in GKE.
@sebright sebright mentioned this pull request Mar 1, 2019
Merged
ahmetb pushed a commit to GoogleCloudPlatform/microservices-demo that referenced this pull request Mar 1, 2019
@sebright @ahmetb
adservice: use Stackdriver special field to set sampling decision in …
d08d419 
…log entries (#168)

This commit sets the new sampling decision field that is recognized by the
Stackdriver Logging agent, "logging.googleapis.com/traceSampled".  The sampling
decision field was added in
GoogleCloudPlatform/fluent-plugin-google-cloud#297, and
it won't be available until the new version of fluent-plugin-google-cloud is
used in GKE.
sebright added a commit to census-ecosystem/opencensus-experiments that referenced this pull request Mar 1, 2019
@sebright
Use Stackdriver special field to set sampling decision in log correla…
3024a0e 
…tion demo. (#179)

This commit sets the new sampling decision field that is recognized by the
Stackdriver Logging agent, "logging.googleapis.com/traceSampled", in the Log4j
log correlation demo.  The sampling decision field was added in
GoogleCloudPlatform/fluent-plugin-google-cloud#297.
@qingling128 qingling128 mentioned this pull request Aug 13, 2019
Closed
neptune-web added a commit to neptune-web/kubernetes-microservices-sam that referenced this pull request Jun 12, 2022
@neptune-web
adservice: use Stackdriver special field to set sampling decision in …
773a591 
…log entries (#168)

This commit sets the new sampling decision field that is recognized by the
Stackdriver Logging agent, "logging.googleapis.com/traceSampled".  The sampling
decision field was added in
GoogleCloudPlatform/fluent-plugin-google-cloud#297, and
it won't be available until the new version of fluent-plugin-google-cloud is
used in GKE.
oplik0 pushed a commit to oplik0/bso that referenced this pull request Jun 6, 2024
@sebright @oplik0
adservice: use Stackdriver special field to set sampling decision in …
3edfb84 
…log entries (#168)

This commit sets the new sampling decision field that is recognized by the
Stackdriver Logging agent, "logging.googleapis.com/traceSampled".  The sampling
decision field was added in
GoogleCloudPlatform/fluent-plugin-google-cloud#297, and
it won't be available until the new version of fluent-plugin-google-cloud is
used in GKE.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igorpeshansky igorpeshansky igorpeshansky approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@qingling128 @igorpeshansky @googlebot