Feature: Dissect processor #6925

ph · 2018-04-23T20:10:12Z

Implement the Dissect Tokenizer from logstash-input-dissect1

This tokenizer allows you to define patterns of strings and extract the
relevant informations. It also permet to do some string manipulations
when extracting the keys.

Example tokenizer:

tokenizer: "%{at} - [%{machine}] %{code} - %{message}"
message: "10/10/2017 - [wopr] 1 - oh fire fire!"
result:
  at: "10/10/2017"
  machine: "wopr"
  code: "1"
  message: "of fire fire!"

tokenizer: "%{?key} %{&key}"
message: "hello world"
result:
  hello: "world"

Example of configuration:

processors:
 - dissect:
    tokenizer: "%{key1} - %{key2}"
    field: "message"
    target_prefix: "extracted"

Dissect support a few more features:

Indirect field
Append
skip field
Greedy padding for CSV file

Performance

When working on that feature I've made some work to reduce the allocation and try to increase the speed, I've defined a few simple regular expression that just do a regexp's match on the data without doing anything else and the dissect is faster and provide more flexibility.

goos: darwin
goarch: amd64
pkg: github.com/elastic/beats/libbeat/processors/dissect
BenchmarkDissect/Complex_stack_trace-8           2000000               607 ns/op             464 B/op          3 allocs/op
BenchmarkDissect/fails_when_delimiter_is_not_found_at_the_beginning_of_the_string-8             2000000000               0.00 ns/op            0 B/op          0 allocs/op
BenchmarkDissect/fails_when_delimiter_is_not_found_after_the_key-8                              2000000000               0.00 ns/op            0 B/op          0 allocs/op
BenchmarkDissect/simple_dissect-8                                                               10000000               199 ns/op             352 B/op          3 allocs/op
BenchmarkDissect/dissect_two_replacement-8                                                       5000000               255 ns/op             368 B/op          3 allocs/op
BenchmarkDissect/one_level_dissect_not_end_of_string-8                                          10000000               233 ns/op             352 B/op          3 allocs/op
BenchmarkDissect/one_level_dissect-8                                                            10000000               209 ns/op             352 B/op          3 allocs/op
BenchmarkDissect/multiple_keys_dissect_end_of_string-8                                           5000000               264 ns/op             368 B/op          3 allocs/op
BenchmarkDissect/multiple_keys_not_end_of_string-8                                               5000000               289 ns/op             368 B/op          3 allocs/op
BenchmarkDissect/simple_ordered-8                                                                3000000               440 ns/op             392 B/op          5 allocs/op
BenchmarkDissect/simple_append-8                                                                 3000000               431 ns/op             392 B/op          5 allocs/op
BenchmarkDissect/indirect_field-8                                                                5000000               269 ns/op             368 B/op          3 allocs/op
BenchmarkDissect/skip_field-8                                                                    5000000               254 ns/op             368 B/op          3 allocs/op
BenchmarkDissect/named_skiped_field_with_indirect-8                                              5000000               298 ns/op             368 B/op          3 allocs/op
BenchmarkDissect/missing_fields-8                                                                3000000               475 ns/op             432 B/op          3 allocs/op
BenchmarkDissect/ignore_right_padding-8                                                          5000000               371 ns/op             384 B/op          3 allocs/op
BenchmarkDissect/padding_on_the_last_key_need_a_delimiter-8                                      5000000               385 ns/op             384 B/op          3 allocs/op
BenchmarkDissect/ignore_left_padding-8                                                           5000000               364 ns/op             384 B/op          3 allocs/op
BenchmarkDissect/when_the_delimiters_contains_`{`_and_`}`-8                                      5000000               316 ns/op             384 B/op          3 allocs/op
BenchmarkDissect/Regular_expression-8                                                            3000000               560 ns/op             240 B/op          1 allocs/op
BenchmarkDissect/Larger_regular_expression-8                                                      500000              3136 ns/op             528 B/op          3 allocs/op
BenchmarkDissect/regular_expression_to_match_end_of_line-8                                       2000000               651 ns/op             304 B/op          3 allocs/op

Note to reviewers:

@dedemorton I will do a followup PR with added documentation.

To understand dissect I suggest either look at the test suite or at the logstash dissect documentation.

ph · 2018-04-23T21:53:24Z

I've talked with @guyboertje and it's not a bug but a feature useful for debugging and making our pattern is right, so to make it work you have to create an empty key at the end to grab the greedy content. I will make the change to this PR so behavior is the same.

This is not true anymore we have changed the behavior so the last key is not greedy.

vjsamuel · 2018-04-24T05:47:47Z

libbeat/processors/dissect/dissect_test.go

+    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
+    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
+    at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61)
+    at java.lang.Thread.run(Thread.java:748)`


@ph i would be interested to know what would be the performance impact of extracting something from the very end of this long string.

Since this code has to read all the bytes to find delimiters and keys the performance will be dependant on the following things:

Length of the string;

Numbers of keys;

appending vs. simple key extraction.

If we take a string as long as the current stack trace to extract one key at the end, assuming we have some repetitive patterns allowing us to create a tokenizer we will have theses results.

With dissect: "MACHINE[%{key}]" => 350 ns/op
Only a regexp match: regexp.MustCompile("MACHINE\\[(\\w+)\\]$") => 656 ns/op

It will still faster, in that case, assuming we have an easy pattern.

Note: in the regexp I am giving a hint that this pattern is a the end of the line, we cannot do that with dissect right now with the current syntax.

ruflin · 2018-04-24T07:36:18Z

libbeat/docs/processors-using.asciidoc

+defined patterns.
+
+
+Note that if the key exists in the `target_field`, the original value will be replaced.


I suggest we don't allow this and someone would have to use drop_field before. This would be the same behaviour as we have in the rename processor.

I am OK with that.

ruflin · 2018-04-24T07:37:20Z

libbeat/docs/processors-using.asciidoc

+
+`field`:: (Optional) The event field to tokenize. Default is `message`.
+
+`target_field`:: (Optional) The name of the field where the values will be extracted. When an empty


It's more the target object or prefix? If I understand this correctly the data will be under extracted.key1, extracted.key2?

@ruflin Yes it is exactly that, it will use another key as the prefix, are you suggesting to rename it to target_prefix or just prefix ?

Yes, as target field suggests to me that is the field it ends up in.

ruflin · 2018-04-24T07:42:19Z

libbeat/processors/dissect/const.go

+	delimiterRE = regexp.MustCompile("(?s)(.*?)%\\{([^}]*?)}")
+	suffixRE    = regexp.MustCompile("(.+?)(/(\\d{1,2}))?(->)?$")
+
+	skipFieldPrefix      = "?"


I wonder if we really need to support all of these prefixes or if we can leave the more complex ones to LS?

Since the syntax is still quite small I prefer we keep the two implementations as close as possible. If we ever make change or additions in the syntax we might discuss it again.

Also the indirect is really useful, take this example:

%{?key} %{&key} -> "hello world" -> {"hello" => "world"}

ruflin · 2018-04-24T07:45:50Z

libbeat/processors/dissect/dissect_test.go

+	"github.com/stretchr/testify/assert"
+)
+
+var export = flag.Bool("dissect-tests", false, "export dissect tests to JSON.")


As this test is in the dissect package I wonder if this flag will show up globally. Probably not as _test files should not be compiled, but better double check.

Its only scoped to this part and won't be present in the end binary.

ph · 2018-04-24T19:41:44Z

libbeat/docs/processors-using.asciidoc

+
+Note that if the key exists in the `target_field`, the original value will be replaced.
+
+TODO: need to finish


@dedemorton Just to keep you in the loop, since both beat and LS will share the same syntax and behavior, should we try to make a high definition/examples of the syntax and share it with between the two project or not?

@ph I think that's a good idea, but there are a few things to consider:

Where will the shared asciidoc source files live? Will there be a new repo for code/docs shared across ingest?

Will the behavior across LS and Beats always be identical? Will the projects share code? If we add a new processor to Beats, will we always add it to Logstash in the same release (and vice versa).

Where will the shared asciidoc source files live? Will there be a new repo for code/docs shared across ingest?

Good question, I think for now we can duplicate the content but have the same description.

Will the behavior across LS and Beats always be identical? Will the projects share code? If we add a new processor to Beats, will we always add it to Logstash in the same release (and vice versa).

For now we only have dissect and in the short term we are not planning anything else.

For now i would keep it separate to make sure it does not hold back the progress of this PR. We can still do a follow up PR / discussion no how to merge it.

@ruflin I will make the docs PR as a separate PR.

Hopefully we can have this feature in Beats first.

@DanielYWoo IIRC Dissect appeared in LS two years ago.

dedemorton · 2018-05-10T15:40:32Z

auditbeat/auditbeat.reference.yml

@@ -250,6 +250,14 @@ auditbeat.modules:
 #       - from: "a"
 #         to: "b"
 #
+# The following example use dissect syntax to tokenize the string into fields:


Can you just say, "The following example tokenizes the string into fields:" That way, if you change the name of the processor someday, you won't have to update the description. :-)

If you feel you need to mention dissect syntax, then say, "The following example uses..." (plural).

dedemorton · 2018-05-10T15:46:10Z

libbeat/docs/processors-using.asciidoc

@@ -761,3 +762,33 @@ The fields added to the event are looking as following:
 -------------------------------------------------------------------------------

 NOTE: The host information is refreshed every 5 minutes.
+
+[[dissect]]
+=== Dissect


We're trying to use goal-oriented titles here. How about:

Dissect strings or Tokenize strings?

I will go with Dissect strings

dedemorton · 2018-05-10T15:50:53Z

libbeat/docs/processors-using.asciidoc

+[[dissect]]
+=== Dissect
+
+The `dissect` processor allows you to use the `dissect` syntax to tokenizer incoming string into


It sounds like you're planning more work for the docs, so I won't make too many comments. But I would shorten this to say:

The dissect processor tokenizes incoming strings....

DanielYWoo · 2018-05-21T07:03:38Z

any update on this? Eager to see it released

ruflin · 2018-05-22T08:17:36Z

libbeat/processors/dissect/dissect_test.go

+}
+
+type dissectTest struct {
+	Name     string `json:"name"`


What is the json part in the struct here for?

Shared tests between LS and Beats see logstash-plugins/logstash-filter-dissect#55

+1 on sharing. Best leave a comment here to the LS code to make this clear as otherwise someone could clean up the code in the future.

ruflin · 2018-05-24T08:38:12Z

libbeat/processors/dissect/config.go

+
+var defaultConfig = config{
+	Field:        "message",
+	TargetPrefix: "extracted",


Should we make it dissect to make it clear it comes from dissect?

ruflin · 2018-05-24T08:38:36Z

libbeat/docs/processors-using.asciidoc

+
+`field`:: (Optional) The event field to tokenize. Default is `message`.
+
+`target_prefix`:: (Optional) The name of the field where the values will be extracted. When an empty


Can you document on what happens if "" is specified and a key already exists?

ruflin · 2018-05-24T08:45:11Z

filebeat/filebeat.reference.yml

+#
+#processors:
+#- dissect:
+#    tokenizer: "%{key1} - %{key2}"


Should we have some guidelines / requirements on what users should use for the keys? Like don't use spaces for example? As dissect allows any match to be a key, this will become interesting.

space is fine, added a note about suffix and prefix modifiers.

ruflin · 2018-05-24T08:49:34Z

libbeat/processors/dissect/dissect_test.go

+}
+
+type dissectTest struct {
+	Name     string `json:"name"`


+1 on sharing. Best leave a comment here to the LS code to make this clear as otherwise someone could clean up the code in the future.

ruflin · 2018-05-24T08:54:45Z

libbeat/docs/processors-using.asciidoc

+`target_prefix`:: (Optional) The name of the field where the values will be extracted. When an empty
+string is defined, the processor will create the keys at the root of the event. Default is
+`extracted`.
+


One thing also missing here: what happens on error if dissect does not match or only a part of it. Is an error reported?

ruflin · 2018-05-24T08:57:26Z

libbeat/processors/dissect/field.go

+	return fmt.Sprintf("field: %s, ordinal: %d, greedy: %v", f.key, f.ordinal, f.IsGreedy())
+}
+
+type normalField struct {


Would be nice to have some docs here or on the top of this file to explain the difference between baseField, normalField, namedSkipField. This should make it easier for someone contributing to the code in the future.

You explain it well in the PR message the different cases but it's not documented in the code.

ruflin · 2018-05-24T09:01:41Z

libbeat/processors/dissect/field.go

+		return rawKey, 0, false
+	}
+
+	if m[0][3] != "" {


When I saw this code my first worry is a panic in case index 3 does not exist. Same for 4 below. Should we have a check or should this never be possible based on the code path?

This is a pretty good question, let me verify that.

The len(m) will take care of unmatched regular expression and the regular expression when matched will always return 4 groups, if no value is found or extracted it will be an empty string.

But the slice will always have 4 elements.

ruflin · 2018-05-24T09:02:41Z

libbeat/processors/dissect/parser.go

+
+	pos := 0
+	for id, m := range matches {
+		d := newDelimiter(tokenizer[m[2]:m[3]])


Same worries as above.

Should be fine for the same reason as above.

ruflin · 2018-05-24T09:06:08Z

libbeat/processors/dissect/processor.go

+
+	for k, v := range m {
+		if _, err := event.GetValue(k); err == common.ErrKeyNotFound {
+			event.PutValue(k, v)


Should we use safemapstr.Put(...) here so people can't shoot in their own foot?

I think in that case its preferable to have an errors and this will wont lead to surprises, IE when you want to use the key for an index name.

ph · 2018-05-24T15:57:52Z

@ruflin I've went through the first round of comments and updated/rebased the code.

ruflin · 2018-05-25T14:07:24Z

Code LGTM. I'm planning to pull the branch and play with the binary beginning next week.

ruflin · 2018-05-28T12:26:45Z

libbeat/processors/dissect/dissect.go

+
+// Raw returns the raw tokenizer used to generate the actual parser.
+func (d *Dissector) Raw() string {
+	return d.Raw()


This should be return d.raw I think. Just tried it to run it locally and got

runtime: goroutine stack exceeds 1000000000-byte limit fatal error: stack overflow runtime stack: runtime.throw(0x4b6f321, 0xe) /usr/local/Cellar/go/1.10.2/libexec/src/runtime/panic.go:616 +0x81 runtime.newstack() /usr/local/Cellar/go/1.10.2/libexec/src/runtime/stack.go:1054 +0x71f runtime.morestack() /usr/local/Cellar/go/1.10.2/libexec/src/runtime/asm_amd64.s:480 +0x89 goroutine 1 [running]: github.com/elastic/beats/libbeat/processors/dissect.(*Dissector).Raw(0xc4204375a0, 0x0, 0x0) /Users/ruflin/Dev/gopath/src/github.com/elastic/beats/libbeat/processors/dissect/dissect.go:48 +0x4e fp=0xc4404a6378 sp=0xc4404a6370 pc=0x484e19e github.com/elastic/beats/libbeat/processors/dissect.(*Dissector).Raw(0xc4204375a0, 0x0, 0x0) /Users/ruflin/Dev/gopath/src/github.com/elastic/beats/libbeat/processors/dissect/dissect.go:49 +0x2b fp=0xc4404a63a0 sp=0xc4404a6378 pc=0x484e17b github.com/elastic/beats/libbeat/processors/dissect.(*Dissector).Raw(0xc4204375a0, 0x0, 0x0)

ruflin · 2018-05-28T12:30:01Z

I tried locally the following config (after applying the fix to Raw() method mentioned above):

processors:
 - dissect:
     tokenizer: "/var/%{prefix}/%{path}"
     field: "source"
     target_prefix: "extracted"

The idea here is to extract information from the file path. This has been a requested feature in the past especially for the docker case (which we solved differently now). Also users requested to get only the file name and not the full path.

The above works as expected on logs coming from /var/log/* and the output contains the following fields:

  "extracted": {
    "prefix": "log",
    "path": "fsck_apfs.log"
  },

@ph I think the Raw() problem is really something that should pop up in our tests. I suggest to add at least 1 or 2 system tests to cover for such cases.

ph · 2018-05-28T13:00:49Z

Correct that should have poped, I think I've changed the name at some point. Let me correct the situation with tests, I should have them today.

On Mon, May 28, 2018 at 8:30 AM Nicolas Ruflin ***@***.***> wrote: I tried locally the following config (after applying the fix to Raw() method mentioned above): processors: - dissect: tokenizer: "/var/%{prefix}/%{path}" field: "source" target_prefix: "extracted" The idea here is to extract information from the file path. This has been a requested feature in the past especially for the docker case (which we solved differently now). Also users requested to get only the file name and not the full path. The above works as expected on logs coming from /var/log/* and the output contains the following fields: "extracted": { "prefix": "log", "path": "fsck_apfs.log" }, @ph <https://github.com/ph> I think the Raw() problem is really something that should pop up in our tests. I suggest to add at least 1 or 2 system tests to cover for such cases. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6925 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAACgB9OiBXPTL32YXos5Fgmzu6A7Buqks5t2-3JgaJpZM4TgilI> .

-- ph

ph · 2018-05-28T16:10:54Z

@ruflin I've fixed the Raw case and also added 2 new system tests for the following scenario.

Good tokenizer => Expect keys to be extracted in a specific field.
Bad tokenizer => Expect no keys to be extracted but the event should not be dropped.

ruflin · 2018-05-29T07:34:42Z

auditbeat/auditbeat.reference.yml

+#- dissect:
+#    tokenizer: "%{key1} - %{key2}"
+#    field: "message"
+#    target_prefix: "extracted"


We have in the code now dissect as default. This should also be adjusted here.

ruflin

One minor comment left. Otherwise LGTM

ruflin · 2018-05-29T07:35:07Z

filebeat/tests/system/test_processors.py

+                "dissect": {
+                    "tokenizer": "\"%{key} world\"",
+                    "field": "message",
+                    "target_prefix": "extracted"


If you leave this one out, you can test the defaults.

I have added something specific to test all the defaults instead, I prefer clearer intend.

ph · 2018-05-29T12:11:39Z

Good comments will make the changes thanks @ruflin

On Tue, May 29, 2018 at 3:36 AM Nicolas Ruflin ***@***.***> wrote: ***@***.**** commented on this pull request. One minor comment left. Otherwise LGTM ------------------------------ In filebeat/tests/system/test_processors.py <#6925 (comment)>: > @@ -115,3 +115,56 @@ def test_condition(self): assert "beat.name" in output assert "message" in output assert "test" in output["message"] + + def test_dissect_good_tokenizer(self): + """ + Check dissect with a good tokenizer + """ + self.render_config_template( + path=os.path.abspath(self.working_dir) + "/test.log", + processors=[{ + "dissect": { + "tokenizer": "\"%{key} world\"", + "field": "message", + "target_prefix": "extracted" If you leave this one out, you can test the defaults. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6925 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAACgHWY9bCH13sf8oRvievAgGAW7Kh8ks5t3Pp6gaJpZM4TgilI> .

-- ph

Implement the Dissect Tokenizer from `logstash-input-dissect`[1] This tokenizer allows you to define patterns of strings and extract the relevant informations. It also permet to do some string manipulations when extracting the keys. Example tokenizer: ```yaml tokenizer: "%{at} - [%{machine}] %{code} - %{message}" message: "10/10/2017 - [wopr] 1 - oh fire fire!" result: at: "10/10/2017" machine: "wopr" code: "1" message: "of fire fire!" ``` ```yaml tokenizer: "%{?key} %{&key}" message: "hello world" result: hello: "world" ``` Example of configuration: ```yaml processors: - dissect: tokenizer: "%{key1} - %{key2}" field: "message" target_field: "extracted" ``` Dissect support a few more features: - Indirect field - Append - skip field - Greedy padding for CSV file [1]: https://github.com/logstash-plugins/logstash-filter-dissect

ph · 2018-05-29T13:21:56Z

@ruflin I did the last changes, I think we are good now, thanks!

ruflin · 2018-05-29T14:32:06Z

libbeat/processors/dissect/config.go

+}
+
+var defaultConfig = config{
+	Field:        "message",


Based on elastic/ecs#3 this could become log.message (not now)

DanielYWoo · 2018-06-01T06:01:45Z

Hi guys, what's the next release date?

tsg added the in progress Pull request is currently in progress. label Apr 23, 2018

vjsamuel reviewed Apr 24, 2018

View reviewed changes

ruflin reviewed Apr 24, 2018

View reviewed changes

ruflin added libbeat :Processors labels Apr 24, 2018

ph commented Apr 24, 2018

View reviewed changes

ph mentioned this pull request May 5, 2018

Feature Request: Allow regular expressions in prospector paths and extract to metadata #7024

Closed

ph force-pushed the feature/dissect-tokenizer branch from 1c83ba4 to c124419 Compare May 9, 2018 15:46

dedemorton reviewed May 10, 2018

View reviewed changes

ph force-pushed the feature/dissect-tokenizer branch from c124419 to ab3b4ba Compare May 17, 2018 15:21

ph added review and removed in progress Pull request is currently in progress. labels May 17, 2018

ruflin reviewed May 22, 2018

View reviewed changes

ruflin reviewed May 24, 2018

View reviewed changes

ph force-pushed the feature/dissect-tokenizer branch from 520028c to b6549fe Compare May 24, 2018 15:57

ruflin reviewed May 28, 2018

View reviewed changes

ruflin reviewed May 29, 2018

View reviewed changes

ph force-pushed the feature/dissect-tokenizer branch from aa49bba to 7bf3b5a Compare May 29, 2018 13:13

ph force-pushed the feature/dissect-tokenizer branch from 7bf3b5a to 8dedd14 Compare May 29, 2018 13:19

ruflin approved these changes May 29, 2018

View reviewed changes

ruflin merged commit 81c8671 into elastic:master May 29, 2018

andrewkroh added the v6.4.0 label Jun 19, 2018

		defined patterns.


		Note that if the key exists in the `target_field`, the original value will be replaced.


		`field`:: (Optional) The event field to tokenize. Default is `message`.

		`target_field`:: (Optional) The name of the field where the values will be extracted. When an empty


		Note that if the key exists in the `target_field`, the original value will be replaced.

		TODO: need to finish


		`field`:: (Optional) The event field to tokenize. Default is `message`.

		`target_prefix`:: (Optional) The name of the field where the values will be extracted. When an empty

Feature: Dissect processor #6925

Feature: Dissect processor #6925

Conversation

ph commented Apr 23, 2018 • edited Loading

Performance

Note to reviewers:

ph commented Apr 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielYWoo commented May 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ph May 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ph May 24, 2018 • edited Loading

Choose a reason for hiding this comment

ph commented May 24, 2018

ruflin commented May 25, 2018

Choose a reason for hiding this comment

ruflin commented May 28, 2018

ph commented May 28, 2018 via email

ph commented May 28, 2018

Choose a reason for hiding this comment

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ph commented May 29, 2018 via email

ph commented May 29, 2018

Choose a reason for hiding this comment

DanielYWoo commented Jun 1, 2018

ph commented Apr 23, 2018 •

edited

Loading

ph commented Apr 23, 2018 •

edited

Loading

ph May 24, 2018 •

edited

Loading

ph May 24, 2018 •

edited

Loading