-
Notifications
You must be signed in to change notification settings - Fork 22
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.
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
Update Realtime Profiling Instance Naming #256
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolve the duplicate test asserts and see what others think about the hostname config. 👍
assertEquals("test_app_name", attributes.asMap().get(SERVICE_NAME)); | ||
assertEquals("test_app_name", attributes.asMap().get(APP_NAME)); | ||
assertEquals("test_hostname", attributes.asMap().get(HOSTNAME)); | ||
assertEquals("test_app_name", attributes.asMap().get(APP_NAME)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks to be a dup of the asserts above (APP_NAME and SERVICE_NAME checks)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
assertEquals("test_app_name", attributes.asMap().get(SERVICE_NAME)); | ||
assertEquals("test_app_name", attributes.asMap().get(APP_NAME)); | ||
assertEquals("resolved_hostname", attributes.asMap().get(HOSTNAME)); | ||
assertEquals("test_app_name", attributes.asMap().get(APP_NAME)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dups
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
assertEquals("test_app_name", attributes.asMap().get(SERVICE_NAME)); | ||
assertEquals("test_app_name", attributes.asMap().get(APP_NAME)); | ||
assertEquals(loopbackAddress.toString(), attributes.asMap().get(HOSTNAME)); | ||
assertEquals("test_app_name", attributes.asMap().get(APP_NAME)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same duplication
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
hostname = InetAddress.getLocalHost().toString(); | ||
} catch (Throwable e) { | ||
hostname = InetAddress.getLoopbackAddress().toString(); | ||
if (config.getHostname() == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about having hostname as a config. Probably be good to get other opinions on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking back, there is no real need to update the config. This value can be accessed and added to the commonAttributes from the Agent, without changing the config at all.
Resolves Issue #1864