Skip to content

Conversation

@eyalkoren
Copy link
Contributor

@eyalkoren eyalkoren commented Jun 11, 2019

No description provided.

@eyalkoren eyalkoren requested review from axw and felixbarny June 11, 2019 12:29
@eyalkoren eyalkoren self-assigned this Jun 11, 2019
jw.writeByte(OBJECT_END);
}

private void serializeStackFrameModule(final String fullyQualifiedClassName) {
Copy link
Member

Choose a reason for hiding this comment

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

What about using the Java 9 module or jar file name instead of the package name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like using the package name aligns better with the majority of other agents (using sort of namespace for module).
@axw WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think the package name aligns better with the other agents.

  • In Python, it's the Python module, which is effectively a namespace. This may be where the field name originated. In Python, there could be multiple modules distributed together in one package (~jar).
  • In Go, it's the package path (again, ~namespace). Again, there could be multiple packages within one module, a relatively new concept to Go. Just to keep us on our toes, the names/relations are flipped from Python.
  • In Node.js, it's the node module name.
  • For Ruby, there's no reported module as it's not readily available.

The Jar/module name could be useful for linking to Code Search, but I would think it'd be more useful for now to report the package name than the jar, since a large project may have multiple packages with the classes/files of the same name.

Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

LGTM, but let's make sure we're all in agreement before merging.

@eyalkoren
Copy link
Contributor Author

let's make sure we're all in agreement before merging

@axw "all" mean- us three, or other agent devs as well?

I thought a package would also be better in terms of consistency. As I said, jars are not something we have at this stage, it would take some effort, for example- creating a file-system-tree-like cache to link a class name to a source jar (probably when loaded). Modules are only relevant to Java 9 (and even then not always?), so package name is probably the most consistent. I just noted that the name module doesn't fit very well, IMO, namespace would be a better fit.
@felixbarny are you OK with using the package name for now?

@axw
Copy link
Member

axw commented Jun 12, 2019

"all" mean- us three, or other agent devs as well?

I just meant us three.

@eyalkoren eyalkoren merged commit 475b883 into elastic:master Jun 12, 2019
@eyalkoren eyalkoren deleted the stack-trace-serialization branch June 12, 2019 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants